-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HystrixObservableCommand does not allow setting thread pool id? #805
Comments
This is an oversight. I'll add this Setter to the constructor of HystrixObservableCommand for the next release. Thanks for the report. |
Even though it is possible, it's generally inappropriate for a HystrixObservableCommand to be using a thread since that kind of defeats the point of avoiding threads and wrapping non-blocking Observables. HystrixObservableCommand is for wrapping non-blocking Observables. If the Observable is blocking, then it is questionable as to why an Observable is being used and thus HystrixCommand is probably the right thing since it is blocking. If there is a legit reason for the Observable to be blocking, and |
Hello Ben, You said that HystrixObservableCommand is for wrapping non-blocking Observables and in our case to achieve that we are using async javax.ws.rs.client and invocation callback in construct method. How hystrix thread isolation can make it blocking or maybe it doesn't fit each other in another aspect? Cheers, Piotr |
A thread doesn't make it blocking. If you already have a non-blocking
Observable, you don't need another thread. It's wasteful as all that will
happen is it will schedule the thread to schedule the async Observable and
that thread will immediately be put back in the pool. It provides no
benefit and is just a resource waste.
If you are wrapping something that is blocking, then use HystrixCommand. If
what you're wrapping is async (meaning you don't need a thread to make it
async), then HystrixObservableCommand is the right solution, and you don't
need a thread.
|
Thank you for your clarification. Btw I was thinking to switch from semaphore to thread isolation to solve my hystrix command maxConcurrentRequests issue, but that was wrong assumption. I have three instances of my service launched on tomcat with hystrix command execution/falback.isolation.semaphore.maxConcurrentRequests property set to 200, and we were not able to acquire semaphore from time to time. From hystrix documentation I see that semaphore should still be a small percentage of the overall container (i.e. Tomcat) thread pool. It probably means I should set less than 200 (cause default tomcat thread pool is 200) and use 7,8 instances then. Cheers, PK |
@abelnation - Did @benjchristensen's clarification answer your question, or are you still in favor of adding this configuration ability? |
Clarified, thanks. |
Perhaps I'm not understanding the difference properly, but it's strange that I cannot set the thread pool key on a
HystrixObservableCommand
. Is that done for any specific reason? ThanksThe text was updated successfully, but these errors were encountered: