Add response-handler executor parameter to internal client actions #86765
Labels
:Core/Infra/REST API
REST infrastructure and utilities
Team:Core/Infra
Meta label for core/infra team
>tech debt
Comments
DaveCTurner
added
:Distributed/Network
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
The es-distributed team discussed this and arrived at requiring an |
DaveCTurner
changed the title
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this issue
Jun 14, 2022
`TaskListener` is almost always a trivial wrapper around an `ActionListener`. There's a couple of places where that's not true, it reports the task ID in a log message, but these logging effects can be achieved in other ways which don't require polluting various widely-used APIs so much. This commit replaces all usages of `TaskListener` with `ActionListener`, reworking the logging effects, and removing the now-unnecessary overrides. Relates elastic#86765 in that this commit simplifies some of the APIs that elastic#86765 will need to adjust.
This was referenced Jun 15, 2022
elasticsearchmachine
pushed a commit
that referenced
this issue
Jun 24, 2022
`TaskListener` is almost always a trivial wrapper around an `ActionListener`. There's a couple of places where that's not true, it reports the task ID in a log message, but these logging effects can be achieved in other ways which don't require polluting various widely-used APIs so much. This commit replaces all usages of `TaskListener` with `ActionListener`, reworking the logging effects, and removing the now-unnecessary overrides. Relates #86765 in that this commit simplifies some of the APIs that #86765 will need to adjust.
DaveCTurner
added
:Core/Infra/REST API
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
This is basically the other half of #97916. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Actions executed via the transport client would fork to the
LISTENERthreadpool to complete their listener to avoid problems with running too much work on a transport thread. In contrast, actions executed internally viaNodeClientdon't fork by default. We removed this forking behaviour entirely in #53049 as part of the removal of the transport client.Since then we've seen at least a couple of issues where internal actions end up inadvertently doing nontrivial work on a transport thread:
I think this is trappy, there's not even anywhere to specify which thread to use to complete the listener so it's understandable if a developer might think it's going to be completed somewhere safe, perhaps on the same threadpool as was used to call the action in the first place.
I think we should at least require an
executorparameter when executing an action via the client so that callers must make a conscious decision about whether to fork for the response listener or not. Alternatively we could just make them all fork by default (to where? reinstate theLISTENERthreadpool?) and allow an opt-in non-forking API for performance-critical cases.The text was updated successfully, but these errors were encountered: