Skip to content
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

Properly handle default values for allowMaximumSizeToDivergeFromCoreSize #1435

Conversation

ptab
Copy link
Contributor

@ptab ptab commented Dec 6, 2016

Properly handle default values for allowMaximumSizeToDivergeFromCoreSize when configuring properties using Archaius.

Fixes #1434

…ize when

configuring properties using Archaius
@ptab
Copy link
Contributor Author

ptab commented Dec 7, 2016

To give a bit more context: the default value for HystrixThreadPoolProperties.allowMaximumSizeToDivergeFromCoreSize is false instead of null, which means

private static boolean getValueOnce(String propertyPrefix, HystrixThreadPoolKey key, String instanceProperty, boolean builderOverrideValue, boolean defaultValue) {
  return forBoolean()
         .add(propertyPrefix + ".threadpool." + key.name() + "." + instanceProperty, builderOverrideValue)
         .add(propertyPrefix + ".threadpool.default." + instanceProperty, defaultValue)
         .build()
         .get();
}

will never try to read the value for threadpool.default.

A workaround for the current code is to either configure it programatically using HystrixThreadPoolProperties.Setter or adding a configuration for every specific threadpool key.

@mattrjacobs
Copy link
Contributor

I just built a jar with this PR and deployed it into an environment with hystrix-1.5.8. It has a regression - when the property is set to false, the default maximum (10) is getting picked up. I'm reviewing the code to determine where that happens.

@mattrjacobs
Copy link
Contributor

@ptab Can you try out #1447 and see if it works for you? It builds on top of this PR (#1435)

It fixes 2 issues:

  1. Maintain invariant of maximumSize == coreSize if allowMaximumSizeToDivergeFromCoreSize == false
  2. Fix Configuration stream so that it also knows about this invariant

@mattrjacobs mattrjacobs merged commit 0c8f733 into Netflix:master Dec 20, 2016
@ptab
Copy link
Contributor Author

ptab commented Dec 21, 2016

I am a bit late to the party, but I did run 1.5.9 against my test case and verified that it works as expected. Thanks for accepting it! 👍

@mattrjacobs
Copy link
Contributor

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants