-
Notifications
You must be signed in to change notification settings - Fork 233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java duration (#694) #706
Java duration (#694) #706
Conversation
…ration` for Java API at Timeout related classes
…ration` for Java API at Timeout related classes
for(i <- 0 until 10) { | ||
policy.execute(timeout => { | ||
for(_ <- 0 until 10) { | ||
policy.execute((timeout: FiniteDuration) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I chaged the blow part:
https://github.com/paypal/squbs/pull/706/files#diff-5a24577de789cacc6fa9751834aa36c3R97
https://github.com/paypal/squbs/pull/706/files#diff-6f9c73a3eba62c02bf35c3d0479a1f1fR23
I have to add type for the timeout
, otherwise it will cause error:
[error] ~/repository/github/squbs/squbs-pattern/src/test/scala/org/squbs/pattern/timeoutpolicy/TimeoutPolicySpec.scala:46:14: ambiguous reference to overloaded definition,
[error] both method execute in class TimeoutPolicy of type [T](f: org.squbs.pattern.timeoutpolicy.TimedFn[T])T
[error] and method execute in class TimeoutPolicy of type [T](f: scala.concurrent.duration.FiniteDuration => T)T
[error] match argument types (Comparable[_ >: java.time.Duration with scala.concurrent.duration.Duration <: Comparable[_ >: java.time.Duration with scala.concurrent.duration.Duration <: java.io.Serializable] with java.io.Serializable] with java.io.Serializable => Unit)
[error] policy.execute(timeout => {
[error] ^
[error] one error found
squbs-ext/src/test/java/org/squbs/streams/circuitbreaker/CircuitBreakerTest.java
Outdated
Show resolved
Hide resolved
squbs-ext/src/test/java/org/squbs/streams/circuitbreaker/CircuitBreakerTest.java
Outdated
Show resolved
Hide resolved
squbs-httpclient/src/test/java/org/squbs/httpclient/ClientFlowTest.java
Outdated
Show resolved
Hide resolved
squbs-pattern/src/main/scala/org/squbs/pattern/timeoutpolicy/TimeoutPolicy.scala
Outdated
Show resolved
Hide resolved
squbs-unicomplex/src/test/java/org/squbs/stream/ProperShutdownStreamJ.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for doing this. My general point is we should use the conversion utility unless we find a good reason not to.
squbs-unicomplex/src/test/java/org/squbs/stream/ProperShutdownStreamJ.java
Outdated
Show resolved
Hide resolved
The time display in milliseconds is a little disturbing to me. Let me think it over and see what I can do. Otherwise I'm all good with this PR. |
assertJmxValue(oNameCB, "MaxResetTimeout", "14 days"); | ||
assertJmxValue(oNameCB, "CallTimeout", "12000 milliseconds"); // 12 seconds | ||
assertJmxValue(oNameCB, "ResetTimeout", "780000 milliseconds"); // 13 minutes | ||
assertJmxValue(oNameCB, "MaxResetTimeout", "1209600000 milliseconds"); // 14 days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually do not like such JMX displays at all. I understand the limitations of java.time.Duration
, however, I think we can do much better. Instead of diluting the check to milliseconds, would it be better to change CircuitBreakerStateMXBeanImpl
? For instance, we can change override def getMaxResetTimeout: String = maxResetTimeout.toString
to override def getMaxResetTimeout: String = maxResetTimeout.toCoarsest.toString
. That would be much more consumable in JMX.
Given, toCoarsest
goes through a few recursions. So it is not totally free or super low cost. Placing this conversion at strategic points is important. Should we do this in the JMX bean? Should we do it at construction of the state object? Which one has the least runtime cost? JMX bean is generally not too bad. But since these are timeouts, the creation of objects that are only done at creation/materialization time is also not a bad choice. @anilgursel, what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree @akara. Creation/materialization time makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just resolved merge conflicts from @jiminhsieh
Looks like there is a good amount of merge conflict with the previously approved PR on Scala 2.11 support. Pulling code down and fixing those conflicts. |
15caaf3
to
4682d93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks for your pull request. Please review the following guidelines.
It's still WIP, but I want to discuss some of the changes.