Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

The get-indices API does some nontrivial work on the master and at high
index counts the response may be very large and could take a long time
to compute. Some clients will time out and retry if it takes too long.
Today this API is not properly cancellable which leads to a good deal of
wasted work in this situation, and the potentially-enormous response is
serialized on a transport worker thread.

With this commit we make the API cancellable and move the serialization
to a MANAGEMENT thread.

Relates #77466

The get-indices API does some nontrivial work on the master and at high index counts the response may be very large and could take a long time to compute. Some clients will time out and retry if it takes too long. Today this API is not properly cancellable which leads to a good deal of wasted work in this situation, and the potentially-enormous response is serialized on a transport worker thread. With this commit we make the API cancellable and move the serialization to a `MANAGEMENT` thread. Relates elastic#77466
@DaveCTurner DaveCTurner added >bug :Data Management/Indices APIs APIs to create and manage indices and templates v7.17.5 v8.4.0 v8.3.1 labels Jun 15, 2022
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Jun 15, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Collaborator

Hi @DaveCTurner, I've created a changelog YAML for you.

@DaveCTurner
Copy link
Contributor Author

Backporting this to 7.17 is debatable, although I think it's reasonable to do so.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks David.
I wonder what other APIs we may need to make this change for?
(currently: cluster state, get snapshot, get mapping, segment stats APIs?)

@DaveCTurner DaveCTurner merged commit eb55b48 into elastic:master Jun 16, 2022
@DaveCTurner DaveCTurner deleted the 2022-06-15-cancellable-get-indices branch June 16, 2022 09:28
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.17 Commit could not be cherrypicked due to conflicts
8.3 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 87681

@DaveCTurner
Copy link
Contributor Author

Thanks Martijn.

I wonder what other APIs we may need to make this change for?

IMO all of them, at least it'd be great for cancellation to be an opt-out thing rather than opt-in. Unfortunately it's a bit hard to determine where to focus our efforts without evidence from users showing that cancellation support is needed, which is what happened here. I count 28 APIs that already support cancellation, which is a good start.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jun 16, 2022
The get-indices API does some nontrivial work on the master and at high index counts the response may be very large and could take a long time to compute. Some clients will time out and retry if it takes too long. Today this API is not properly cancellable which leads to a good deal of wasted work in this situation, and the potentially-enormous response is serialized on a transport worker thread. With this commit we make the API cancellable and move the serialization to a `MANAGEMENT` thread. Backport of elastic#87681 Relates elastic#77466
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jun 16, 2022
The get-indices API does some nontrivial work on the master and at high index counts the response may be very large and could take a long time to compute. Some clients will time out and retry if it takes too long. Today this API is not properly cancellable which leads to a good deal of wasted work in this situation, and the potentially-enormous response is serialized on a transport worker thread. With this commit we make the API cancellable and move the serialization to a `MANAGEMENT` thread. Backport of elastic#87681 Relates elastic#77466
elasticsearchmachine pushed a commit that referenced this pull request Jun 16, 2022
The get-indices API does some nontrivial work on the master and at high index counts the response may be very large and could take a long time to compute. Some clients will time out and retry if it takes too long. Today this API is not properly cancellable which leads to a good deal of wasted work in this situation, and the potentially-enormous response is serialized on a transport worker thread. With this commit we make the API cancellable and move the serialization to a `MANAGEMENT` thread. Backport of #87681 Relates #77466
elasticsearchmachine pushed a commit that referenced this pull request Jun 16, 2022
The get-indices API does some nontrivial work on the master and at high index counts the response may be very large and could take a long time to compute. Some clients will time out and retry if it takes too long. Today this API is not properly cancellable which leads to a good deal of wasted work in this situation, and the potentially-enormous response is serialized on a transport worker thread. With this commit we make the API cancellable and move the serialization to a `MANAGEMENT` thread. Backport of #87681 Relates #77466
@martijnvg
Copy link
Member

Backporting this to 7.17 is debatable, although I think it's reasonable to do so.

I forgot to mention, but yes, I think this change also qualifies for backporting to 7.17.

IMO all of them, at least it'd be great for cancellation to be an opt-out thing rather than opt-in.

+1

Unfortunately it's a bit hard to determine where to focus our efforts without evidence from users showing that cancellation support is needed, which is what happened here. I count 28 APIs that already support cancellation, which is a good start.

YMaybe we should have a meta issue for this. I will ask around whether this is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Data Management/Indices APIs APIs to create and manage indices and templates Team:Data Management Meta label for data/management team v7.17.5 v8.3.1 v8.4.0

4 participants