Implementation for feature request #1013

[tine2k]

I implemented a simple way to configure the timeout thread pool core size via a system property. The default is still Runtime.getRuntime(). availableProcessors(). I also added test cases for this feature.

[cloudbees-pull-request-builder]

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

[mattrjacobs]

This looks like a good change. The only concern I have is that all other properties offer Archaius integration, which allows for on-the-fly reconfiguration. Is that something you're willing to look at?

[tine2k]

I have looked into this. There is only one TimerThreadPool (HystrixTimer) that handles timeouts for multiple ThreadPools based on HystrixThreadPoolKey. Putting this setting in the HystrixThreadPoolProperties class would therefore not a be clean solution. This setting is also neither a CommandProperty nor a CollapserProperty.
If we want to integrate this setting with the current design, it would be best in a new fourth type of properties. We could either do this in form of a HystrixGeneralProperties class that contains configuration that is independent of any thread pools or command. A second option would be to create something like a TimerThreadPoolProperties class which contains only TimerThreadPool specific config.
I will implement this. Please let me know, which option you prefer.

[lixlix]

I just ran into that issue. What's the rationale behind setting the number of threads in the TimerThreadPool to the number of available processors?

[mattrjacobs]

@tine2k My personal preference is TimerThreadPoolProperties. I generally dislike catch-all classes like HystrixGeneralProperties, as it's hard to understand what gets contained in a class like that without diving in. Naming a class TimerThreadPoolProperties lets users know the precise sets of things that may be configured: commands/threadpools/collapsers/timer thread pool

WDYT?

[mattrjacobs]

@lixlix Good question. I didn't make that design choice initially, and it's not documented either. My guess would be that this choice was made based on the observation that work done by the system should be scaling roughly by the number of CPUs, and so the timer threads should scale with this value as well.

Mostly due to the fact there's not a configuration knob, I haven't experimented with different values for the thread pool size. Do you have any insight on a good mechanism to choose the timer thread pool size?

[lixlix]

@mattrjacobs No insight on my side - that was why I was asking for the design decision behind that. Scaling that way doesn't make too much sense, I think.

[tine2k]

@mattrjacobs I agree. TimerThreadPoolProperties it will be. I will work this in.

@mattrjacobs @lixlix I don't think this is a good default either. If I understand the mechanism correctly, the timer thread pool is for handling timeouts of actions until the circuit opens. If the circuit is open, actions will fail right away and the timer thread pool is no longer used. Therefore, you require a large timer thread pool only if you have a lot of timeouts to deal with (e.g. if circuit breaking is disabled). Furthermore, timeout handling is a cheap operation compared to the usual HystrixCommands because there is no IO involved (right?).
I suggest we use a flat default (like 5 or so) and offer a way to configure it. If we want a scaling default, it should scale along the size and count of thread pools used for HystrixCommands.

[mattrjacobs]

I don't disagree with the flat number of threads approach. I'd like to do some experiments (both using jmh and Netflix production apps) to see what happens when we limit the threads.

I also wanted to point out that Hystrix collapsing uses the HystrixTimer threads. Those threads generate ticks and then performing the batching

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #301 FAILURE
Looks like there's a problem with this pull request

[tine2k]

@mattrjacobs I updated the branch with a HystrixTimerThreadPoolProperties implementation. Please review. I left the default as it is. Feel free to change to a flat number after your experiments.

[cloudbees-pull-request-builder]

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

[mattrjacobs]

Unused import

[mattrjacobs]

I left a couple of small cosmetic comments, but this looks great! Thanks for doing the extra work to get Archaius integration.

[mattrjacobs]

Merging. Will issue the cleanup as a separate commit. Thanks, @tine2k!

[tine2k]

Thanks for your review notes, @mattrjacobs. Sorry, I couldn't work them in in time (I would have). Anyways, thanks for the merge!