Use batched executor for shutdown node actions #86018
Conversation
The transport actions for put and delete of the node shutdown api currently use an unbatched executor. This commit converts the two classes to each use a batched executor. closes elastic#84847
rjernst
added
>refactoring
:Core/Infra/Node Lifecycle
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Note: this can likely be simplified further, sharing a single executor across both put and delete, but I think that is probably beyond the scope of this PR. |
| taskContext.onFailure(e); | ||
| continue; | ||
| } | ||
| var reroute = taskContext.getTask().rerouteService(); |
There was a problem hiding this comment.
Why pass in the reroute service through the task? It is available from ClusterService#getRerouteService at construction time, and I was expecting it to be captured as a field on the executor.
There was a problem hiding this comment.
Then the executor would not be static. I don't feel strongly one way or another, but I was trying to make the executor stateless (then eventually it could work across both delete and put actions).
There was a problem hiding this comment.
Ok I'd prefer to capture the reroute service on the executor. There's no real need to re-use the same executor instance across both kinds of action - in practice we'll eventually be able to safely batch tasks across executors if they're written in this style.
| continue; | ||
| } | ||
| var reroute = taskContext.getTask().rerouteService(); | ||
| taskContext.success(taskContext.getTask().listener().delegateFailure((l, s) -> ackAndReroute(request, l, reroute))); |
There was a problem hiding this comment.
Observation: this is definitely emerging boilerplate:
taskContext.success(taskContext.getTask().listener().delegateFailure((l, s) -> ...
At the moment not every task has an underlying listener() so we can't yet do this in a generic way, but many tasks do and the remainder should be fixable. I hope to be able to address that to the point where this noise goes away.
There was a problem hiding this comment.
The whole thing feels like boilerplate to me. 😄 I think with a nice base task class everything could be extracted out so this pattern is solidified and a common executor implementation can be used (at least in most cases).
The transport actions for put and delete of the node shutdown api
currently use an unbatched executor. This commit converts the two
classes to each use a batched executor.
closes #84847