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

Is it necessary always to set the core pool size in HystrixThreadPoolDefault.touchConfig() #1245

Closed
yanglifan opened this issue Jun 14, 2016 · 4 comments

Comments

@yanglifan
Copy link
Contributor

While creating a HystrixCommand, a thread pool will be acquired and HystrixThreadPoolDefault.touchConfig() will be executed by default. In HystrixThreadPoolDefault.touchConfig(), threadPool.setCorePoolSize(dynamicCoreSize) will be executed.

In Java 6, threadPool.setCorePoolSize(dynamicCoreSize) will execute a lock operation all the time. That is negative for the performance and sometimes in the high load scenario will cause a live lock issue.

In Java 7 & 8, if the max size is greater than the core size (it is not the default scenario, but someone can have the custom thread pool), the idle worker threads will be interrupted. I think it is unnecessary if the core pool size is not changed.

So I think before execute setCorePoolSize, the new core pool size should be compared the original one to decide the necessity of the execution of setCorePoolSize

@mattrjacobs
Copy link
Contributor

I didn't know either of those facts - thanks for bringing this up! Your proposal sounds like a good one. Are you interested in submitting a PR? I'm happy to review, if so. Otherwise, I can get to it before I cut the next release.

@yanglifan
Copy link
Contributor Author

I have created a PR

@yanglifan
Copy link
Contributor Author

The change was very simple but it caused the CI to be failed. I will check the reason, but it will be great if you could have a look.

@mattrjacobs
Copy link
Contributor

Merged PR, looking at flaky unit tests now

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

No branches or pull requests

2 participants