NetflixOSS » Hystrix » Hystrix-pull-requests #219 SUCCESS
This pull request looks good

In terms of possibly blowing up more intelligently I would maybe create a HystrixRequestLog for when the context is not initialized that no-ops when addExecutedCommand is called and throws IllegalStateException when data is accessed when it should not be. Then maybe in some future world if those methods have segregated interfaces then that could put a decent boundary between HystrixCommand and RequestContext

Thanks @mwhipple. I'm going to try to summarize your changes, to make sure I understand what you've done and the implications. Please correct anything I get wrong.

There are 4 cases:

1. No concurrency strategy supplied, HystrixRequestLog or request caching are on (default)
2. No concurrency strategy supplied, no request functionality needed
3. Custom concurrency strategy supplied, HystrixRequestLog or request caching is on (default)
4. Custom concurrency strategy supplied, no request functionality supplied

I've added unit tests in #953 that demonstrate the custom concurrency cases.

In master without your PR, the situation is as follows:

1. HystrixConcurrencyStrategyDefault gets supplied on the first lookup to resolve HystrixConcurrencyStrategy. If an initialized context is found, it is used for the request log. If not, then the request log is null for the command.
2. HystrixConcurrencyStrategyDefault gets supplied on the first lookup to resolve HystrixConcurrencyStrategy. The request log is unconditionally null for the command, since the request log is unwanted.
3. The custom HystrixConcurrencyStrategy is found when accessing it for the first time and cached for the JVM lifetime. HystrixRequestContext.initializeContext() and shutdown() must be called in order for command construction to succeed. Without that, the call to resolve HystrixRequestLog.getCurrentRequest(concurrencyStrategy) throws an exception.
4. The custom HystrixConcurrencyStrategy is found when accessing it for the first time and cached for the JVM lifetime. The call to resolve HystrixRequestLog.getCurrentRequest(concurrencyStrategy) would throw an exception but it is never made, since the requestLog functionality is off. The currentRequestLog is null.

After your PR:

1. Same as above
2. Same as above
3. Same as 1 above
4. Same as 2 above

If I'm interpreting this correctly, this basically drops the distinction between custom and default concurrency strategies. The only important bit is if the request context has been initialized. If it has been, then objects that need request-scoped access (like HystrixRequestLog) work properly. Otherwise, they are null.

On the compatibility side, I think this should never change the result of any method call that was already returning a value. It will now return a null instead of throwing an IllegalStateException in cases where the HystrixRequestContext is null for 2 cases:

1. Computing currentRequestLog at command-construction time when HystrixRequestContext is null and custom concurrency strategy is used
2. Static access to HystrixRequestLog.getCurrentRequest() when HystrixRequestContext is null

The 3 method calls that were throwing exceptions in #953 all return null instead with your change.

The downside to this change is that the immediate feedback of the IllegalStateException is missing when using a custom concurrency strategy, request log is enabled, and the request context is missing.

This is also the upside, as it allows forward progress when request log is only enabled by default, and your custom concurrency strategy doesn't care about request context, and you've not initialized it.

On balance, this seems like a good change and a good opportunity to document all of the above.

Anything I missed, @mwhipple / @amitcse / @ruhkopf / @benjchristensen ?

That all sounds right.

Removing the distinction from the default strategy and a custom one was one of the motivations so that the default strategy could serve as more of a direct template for custom implementations without lurking variations in behavior.

A potentially significant implied variable in the scenarios outlined above is the requestLog.enabled property (which I needed to be reminded of when reading it). That means that the pre-PR lack of exception in 4 requires the explicit disabling of RequestContext features (explicit is always nice but could leave a plugin less immediately pluggable).

Thanks

@mwhipple Yeah, the last issue you brought up has come up in a few threads (#92 / #724 / #718). In order to use a custom concurrency strategy and not initialize the context, the only solution was to explicitly turn off the request log. And that's a pretty hacky solution.

So I think this PR looks good, but I will give others a chance to weigh in. If I don't hear anything negative by the EOD, I'll merge.

Thanks for this, @mwhipple

Thanks @mwhipple !