Add a new interface, DefaultRequestLogStrategy, to disable requestLog for the concurrency strategy.
#718
Conversation
…og for the concurrency strategy. This adds an opt-in interface to allow concurrency strategies to behave like the default concurrency strategy.
|
NetflixOSS » Hystrix » Hystrix-pull-requests #64 FAILURE |
|
If backwards compatibility was broken as this seems to say then we need to fix that. |
|
Hey @benjchristensen it's not clear to me how the change would have affected https://github.com/Netflix/Hystrix/blob/master/hystrix-contrib/hystrix-rx-netty-metrics-stream/src/test/java/com/netflix/hystrix/HystrixCommandMetricsSamples.java#L53 ? |
|
Looking around it seems "Bad method call from inside of a branch" is a problem with java8's verification |
|
@benjchristensen to prove that the test failure was a change rather than my PR, I created a dummy PR which just added a newline to a readme file (#721). Same error, in the same location. Is it perhaps an upgrade to jdk 7u65? (https://bugs.openjdk.java.net/browse/JDK-8051012) (Having jenkins display the java version would be helpful in debugging these things.) |
|
To make sure I understand this change, I'll restate it. Please correct anything I get wrong. In 1.3.x, there was no caching per-command of the HystrixRequestLog in the HystrixCommand, instead it got looked up from the HystrixRequestVariableHolder each time it was referenced. Furthermore, this reference was wrapped in a check to see if the requestLog was enabled. In 1.4.x, upon command construction, the In both 1.3.x and 1.4.x, there is a branch on the type of concurrency strategy. The first case is that the concurrency strategy is the default concurrency strategy. If this is true, then a check is done to see if a The second case is that a custom concurrency strategy is provided. In this case, then a lookup is done in the request cache for the request log. This assumes that a So in order for command construction not to fail, it's necessary for that concurrency strategy to perform the setup/teardown of the request context. Did I get that right? |
|
In light of the above, your change is completely valid. It does force a concurrency provider to know about and use the DefaultRequestLogStrategy. Another change that seems like it should work is to wrap all of the logic which sets the Does that solution sound workable to you? |
|
@mattrjacobs you have that right. I'm happy with either reverting the previous change to guard the entire request log initialization with the property lookup, or the change in this PR. The reason for the strategy in this PR was that it's completely backwards compatible. By opting for a new interface over a method it shouldn't even require a compilation of dependencies built on prior versions of hystrix. I guess I assume that the initialization in the creation of a command removed its guard on purpose.. maybe something in light of the observer based commands? I'm not entirely sure how the HystrixRequestContext works in the observer framework if the actions could occur in different threads, but maybe that's why it forces it to initialize so that the command will always have a reference to the request log thread. |
|
Your point on backwards-compatibility is a good one. However, I have a preference for adding the check back, because it places less burden on a plugin writer. My guess is this was an unintentional change, and hadn't affected any concurrency plugins that we've used in practice, so we didn't spot the regression. It also points out that I should add some unit tests around concurrency plugins without a request-context. I'm guessing there aren't any of those, or else this regression would have been caught. Can you resubmit with the requestLogEnabled check? I'll merge that and release 1.4.2. |
|
Superseded by #724 |
In 706a0f2, the removal of checking of the request log is enabled before the request log initialization has made it impossible to override the concurrency strategy without wrapping all creation/execution of hystrix commands in hystrix context initializations.
This change adds an opt-in interface to use the default request log behavior, instead of forcing the implementor of a concurrency strategy to also change the request log behavior. The advantage of a separate interface is that this is class-file backwards compatible - already compiled concurrency strategies will still work with this commit and will retain the current behavior.
@mattrjacobs