Cleanup Various Uses of ActionListener #40126
Conversation
* Use shorter `map`, `runAfter` or `wrap` where functionally equivalent to anonymous class * Use ActionRunnable where functionally equivalent
original-brownbear
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
| listener.onFailure(e); | ||
| } | ||
| }); | ||
| rewriteShardRequest(request, ActionListener.map(listener, r -> executeDfsPhase(r, task))); |
There was a problem hiding this comment.
I recently stumbled upon a subtle difference between ActionListener.wrap (called by ActionListener.map) compared to how we generally instantiate an action listener like here: there are places where we don't call onFailure in case onResponse throws (here we do , so everything is fine). This has subtle consequences depending on how the listener is used: say you use a CountDown and wait for a response from N nodes, if you call countDown on both onFailure and onResponse, you may end up counting down twice for the same node in case anything goes wrong while executing onResponse. Maybe an edge case, but it may be easy to miss when moving over to wrap or map whenever we wait for a response from multiple nodes.
|
thanks @dnhatn ! |
…der-permit-primary-mode-only * elastic/master: Validate non-secure settings are not in keystore (elastic#42209) Cleanup Various Uses of ActionListener (elastic#40126) Update api links per context (elastic#42033)
* Cleanup Various Uses of ActionListener * Use shorter `map`, `runAfter` or `wrap` where functionally equivalent to anonymous class * Use ActionRunnable where functionally equivalent
* Cleanup Various Uses of ActionListener * Use shorter `map`, `runAfter` or `wrap` where functionally equivalent to anonymous class * Use ActionRunnable where functionally equivalent
map,runAfterorwrapwhere functionally equivalent to anonymous class