Allow multiple values per request-arg in a HystrixObservableCollapser #881
Conversation
|
NetflixOSS » Hystrix » Hystrix-pull-requests #163 SUCCESS |
|
The above unit tests have now been added, which found some unhandled cases in HystrixObservableCollapser. As soon as I get feedback on the overall approach and names for the methods I'm adding, I'll merge and cut release from the 1.4.x branch |
|
NetflixOSS » Hystrix » Hystrix-pull-requests #164 SUCCESS |
| */ | ||
| @Override | ||
| public void call(Subscriber<? super T> observer) { | ||
| responseSubject.onBackpressureBuffer().subscribe(observer); |
There was a problem hiding this comment.
I don't think onBackpressureBuffer is needed here since you're using an unbounded ReplaySubject already.
There was a problem hiding this comment.
I think you can also use unsafeSubscribe to avoid an allocation, as I don't think you need the unsubscribe behavior on termination, nor do you need error handling, as long as you control the Subscriber used here and don't do anything wrong in it.
|
The changes seem good to me. Good unit test coverage. Perhaps this should be canaried before release though to ensure both functionality and performance has not impacted existing high-throughput use cases since this changes the guts of a hot code path? |
|
Will make the changes above, then vet them with a canary in the Netflix environment. There are also no existing perf tests on collapsing. I'll create an issue to get some jmh coverage added |
|
NetflixOSS » Hystrix » Hystrix-pull-requests #165 SUCCESS |
|
I see a small performance degradation, so I'll spend some time on getting jmh in place to confirm that before I merge. |
|
Closed in favor of #890 |
Addresses #865
I'm not ready to merge this yet, but wanted a few eyes on the general approach.
/cc @benjchristensen
Prior to this fix, there was a single-valued ResponseHolder object that prevented multiple values from being emitted per-request argument. Now the stream of values is directly represented as a Subject, which may be subscribed to.
Within CollapsedRequestObservableFunction, the semantics of
setResponsemay not change, as this is used in user-defined HystrixCollapsers already. Therefore, I was forced to addemitResponseandsetCompletemethods to allow multiple values to flow through. These names will be public, so any thoughts on naming would be helpful.Before I merge, I'll beef up test coverage on this class to include tests for :