Serialize Get Mappings Response on Generic ThreadPool#57937
Serialize Get Mappings Response on Generic ThreadPool#57937original-brownbear merged 2 commits intoelastic:masterfrom original-brownbear:run-get-mapping-rest-on-generic
Conversation
For large responses to the get mappings request, the serialization to XContent can be extremely slow (serializing mappings is expensive since we have to decompress and deserialize the mapping source). To not introduce instability on the IO thread handling the get mappings response we should move the serialization to the generic pool. The trade-off of introducing one or two new context switches for responses that are small enough to not cause trouble on the transport thread to prevent instability in case of a large number of mappings in the cluster seems worth it.
|
Pinging @elastic/es-distributed (:Distributed/Network) |
|
I wonder if it would make sense to add a new method to |
Not sure that's necessarily where we'd want to add this given how we already have infrastructure like |
|
We discussed this during team discuss and decided to check if slowness in this API call happens on Cloud at any non-trivial rate. I investigated this on Cloud and we have a lot of slow get mappings responses (up to minutes in response time) so we should take action here I think. |
DaveCTurner
left a comment
There was a problem hiding this comment.
I'd rather not use the generic pool for this, but MANAGEMENT seems like a reasonable option. See inline comment about a timeout.
| // on an IO thread | ||
| threadPool.generic().execute(ActionRunnable.wrap(this, l -> new RestBuilderListener<GetMappingsResponse>(channel) { | ||
| @Override | ||
| public RestResponse buildResponse(final GetMappingsResponse response, final XContentBuilder builder) throws Exception { |
There was a problem hiding this comment.
WDYT about checking for a timeout again here before we do all the serialisation work? If we use a bounded threadpool then these things might pile up, so aborting early might be a helpful way to push back.
|
Jenkins run elasticsearch-ci/packaging-sample-windows |
|
Thanks David! |
For large responses to the get mappings request, the serialization to XContent can be extremely slow (serializing mappings is expensive since we have to decompress and deserialize the mapping source). To not introduce instability on the IO thread handling the get mappings response we should move the serialization to the management pool. The trade-off of introducing one or two new context switches for responses that are small enough to not cause trouble on the transport thread to prevent instability in case of a large number of mappings in the cluster seems worth it.
…62753) Currently, `finishHim` can either execute on the specified executor (in the less likely case that the local node request is the last to arrive) or on a transport thread. In case of e.g. `org.elasticsearch.action.admin.cluster.stats.TransportClusterStatsAction` this leads to an expensive execution that deserializes all mapping metadata in the cluster running on the transport thread and destabilizing the cluster. In case of this transport action it was specifically moved to the `MANAGEMENT` thread to avoid the high cost of processing the stats requests on the nodes during fan-out but that did not cover the final execution on the node that received the initial request. This PR adds to ability to optionally specify the executor for the final step of the nodes request execution and uses that to work around the issue for the slow `TransportClusterStatsAction`. Note: the specific problem that motivated this PR is essentially the same as #57937 where we moved the execution off the transport and on the management thread as a fix as well.
…62753) (#62955) Currently, `finishHim` can either execute on the specified executor (in the less likely case that the local node request is the last to arrive) or on a transport thread. In case of e.g. `org.elasticsearch.action.admin.cluster.stats.TransportClusterStatsAction` this leads to an expensive execution that deserializes all mapping metadata in the cluster running on the transport thread and destabilizing the cluster. In case of this transport action it was specifically moved to the `MANAGEMENT` thread to avoid the high cost of processing the stats requests on the nodes during fan-out but that did not cover the final execution on the node that received the initial request. This PR adds to ability to optionally specify the executor for the final step of the nodes request execution and uses that to work around the issue for the slow `TransportClusterStatsAction`. Note: the specific problem that motivated this PR is essentially the same as #57937 where we moved the execution off the transport and on the management thread as a fix as well.
Solves/mitigates the issue observed in #57284
For large responses to the get mappings request, the serialization
to XContent can be extremely slow (serializing mappings is expensive since
we have to decompress and deserialize the mapping source).
To not introduce instability on the IO thread handling the get mappings response
we should move the serialization to the generic pool.
The trade-off of introducing one or two new context switches for responses that are
small enough to not cause trouble on the transport thread to prevent instability
in case of a large number of mappings in the cluster seems worth it.
Marking this team discuss for now, since we had the larger discussion of where REST actions should execute before and didn't come to a conclusion here yet. While this is a special case, we have a number of other potentially slow REST actions like this one where a very large response gets serialized.