HystrixPropertiesFactory cache silently obviates dynamic properties

[wg-tsuereth]

I have a dynamic source of configuration properties (Consul, in this case). When I construct a new Hystrix command, I feed this constructor the value of a particular property at that moment in time, using a HystrixCommandProperties.Setter and the "Instance Default" syntax documented here: https://github.com/Netflix/Hystrix/wiki/Configuration

For example:

HystrixObservableCommand.Setter setter = [...]
HystrixObservableCommand<...>(setter.withGroupKey(commandGroup).andCommandPropertiesDefaults(
  HystrixCommandProperties.Setter().withExecutionIsolationSemaphoreMaxConcurrentRequests(
    config.getInteger(PROPERTY_NAME, PROPERTY_DEFAULT)
  )
))

What I expect from this usage is that I can change the value at PROPERTY_NAME and new commands will be created with the max concurrent requests limit of the new value. However, in practice, a property cache in Hystrix prevents this value from being updated after the first command of this type is instantiated.

This is due to aggressive caching in HystrixPropertiesFactory.getCommandProperties -- the cache key is built (in HystrixPropertiesStrategy.getCommandPropertiesCacheKey) from the command's key name, alone. So the existence of a cache entry from my first construction of a command of this type means that the properties cache will be re-used for every other command of that type, regardless of what different properties I may be attempting to give it.

The cache key should probably include the customizations I've applied to the HystrixCommandProperties.Setter in the command constructor, so that cache hits only occur when those customizations have not changed.

[wg-tsuereth]

N.B. that this limitation does not apply when Hystrix can load configuration from Archaius V1 (see also #1248).

[agentgt]

This is because you are most likely implementing the wrong plugin. You want to implement HystrixDynamicProperties. See your other referenced bugs for explanation.

[wg-tsuereth]

This is correct for my use-case (and I presume HystrixDynamicProperties is how the Archaius integration is working as well).

However, I still think that this behavior is counterintuitive (and I would argue, incorrect). For example, if I do this:

System.setProperty("some hystrix property", "value A");
commandA = HystrixObservableCommand(...);
System.setProperty("some hystrix property", "value B");
commandB = HystrixObservableCommand(...);

...then command B will have "value B" for its property, as expected.

However, using the Setter syntax, the property key cache gets in the way:

commandA = HystrixObservableCommand(setter.andCommandPropertiesDefaults(...Setter().withSomeHystrixProperty("value A")));
commandB = HystrixObservableCommand(setter.andCommandPropertiesDefaults(...Setter().withSomeHystrixProperty("value B")));

...then command B will have "value A" for this property.

[mattrjacobs]

@wg-tsuereth The reason for this behavior is that we want the configuration to be per-HystrixCommandKey. If HystrixCommand instances with the same HystrixCommandKey had different configuration, then looking at aggregate metrics (at the granularity of a HystrixCommandKey) would require a lot more care.

I agree that the silent discarding of properties is weird, but I think the consistency of having a common set of configuration per-CommandKey outweighs the benefits of allowing arbitrary config

[wg-tsuereth]

Is this true of the other HystrixCommand.Setter fields as well? (Thread pool settings)

If it is the intent to keep properties the same within a command key, then this Setter syntax - in which both the command key, and these properties, are passed at the same time - seems wasteful (and misleading). Essentially what Hystrix is doing is registering property values to the command key, rather than to the command that is being constructed. The HystrixCommand interface doesn't really convey this idea.

[mattrjacobs]

Yes, this is true of all the configuration wiring.

I agree with you that the current design provides the wrong impression to users. Putting the config on the key is an interesting idea that might allow us to somewhat gracefully transition users off the current approach. I will set up a new issue to track that change and outline how that might work.

[mattrjacobs]

Tracked in Issue #1287