Use RelaxedSingleResultDeduplicator in TransportGetAllocationStatsAction
#127121
Conversation
The current SingleResultDeduplicator will run the computation action again, in the same thread, if other threads call execute() while the current computation is running. For a potentially expensive action this can delay the response to the original caller by an additional action execution time. Since we are caching in TransportGetAllocationStatsAction it also is inconsistent with the strict requirements in the current SingleResultDeduplicator, since we can potentially return a response that was calculated before the call to execute(). Note also that due to the recursive nature of the current SingleResultDeduplicator implementation it is possible to continuously delay the original thread if additional threads call execute() while these computations run. This change refactors SingleResultDeduplicator into an interface with two implementations, a strict form which is the same as the original SingleResultDeduplicator, and a relaxed version that completes all waiting listeners, along with the original call's listener, with the single computation result.
JeremyDahlgren
added
>non-issue
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
For a potentially expensive action this can delay the response to the original caller by an additional action execution time.
In practice all the usages today look to fork the expensive work to a background thread, freeing up the original thread to complete its listeners straight away. We should probably document that this is the expected usage pattern.
we can potentially return a response that was calculated before the call to
execute()
Only in the case of TransportGetAllocationStatsAction, and this is deliberate.
I'm not 100% convinced we need to do this, particularly since we're only using this abstraction in one place. If we do need TransportGetAllocationStatsAction to behave differently, maybe we should just do something with our bare hands there for now, until we see some other spot where this abstraction is useful.
| // The first thread will block until all the other callers have added a waiting listener. | ||
| safeAwait(countDownLatch); |
There was a problem hiding this comment.
This is ok here but we should also have tests that don't orchestrate the order of events quite so carefully so that we can be more confident there's no races that might lead to lost listeners or duplicate concurrent executions. Admittedly the testing of SingleResultDeduplicator today is rather weak in this area, I'd be happy to see some more stringent testing of its invariants under concurrent use too.
| })); | ||
| } | ||
|
|
||
| private static class ActionListenerList<T> implements ActionListener<T> { |
There was a problem hiding this comment.
WDYT about just using SubscribableListener here? It uses a linked list so slightly more expensive in general I guess but also more efficient in the common one-subscriber case.
There was a problem hiding this comment.
Yes, makes sense. This is trimmed down quite a bit and as you mentioned this code could just be used directly as needed in TransportGetAllocationStatsAction. I'm fine with closing this and not touching SingleResultDeduplicator. I can create a separate PR to update the class javadoc for SingleResultDeduplicator with the expected usage pattern, or revert changes here and just make that javadoc update.
3 participants
Came across this while working on issue 123248 and was seeing duplicate calls in the stats service (when caching was disabled).
The current
SingleResultDeduplicatorwill run the computation action again, in the same thread, if other threads callexecute()while the current computation is running. For a potentially expensive action this can delay the response to the original caller by an additional action execution time. Since we are caching inTransportGetAllocationStatsActionit also is inconsistent with the strict requirements in the currentSingleResultDeduplicator, since we can potentially return a response that was calculated before the call toexecute().Note also that due to the recursive nature of the current
SingleResultDeduplicatorimplementation it is possible to continuously delay the original thread if additional threads callexecute()while these computations run.This change refactors
SingleResultDeduplicatorinto an interface with two implementations, a strict form which is the same as the originalSingleResultDeduplicator, and a relaxed version that completes all waiting listeners, along with the original call's listener, with the single computation result. This change will be used in the cancellation support that will be added for 123248.