Make GetSnapshotsAction Cancellable #72644
Conversation
If this runs needlessly for large repositories (especially in timeout/retry situations) it's a significant memory+cpu hit => made it cancellable like we recently did for many other endpoints.
original-brownbear
added
>enhancement
:Distributed Coordination/Snapshot/Restore
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| getSnapshotsRequest, | ||
| // no need to fork here, this will be called on the generic pool anyway if its a large response for more than the | ||
| // currently running snapshots | ||
| new DispatchingRestToXContentListener<>(EsExecutors.DIRECT_EXECUTOR_SERVICE, channel, request)); |
There was a problem hiding this comment.
Dispatching to EsExecutors.DIRECT_EXECUTOR_SERVICE seems a bit weird. Is this because RestToXContentListener doesn't check for cancellation before calling toXContent()? If so, can we address that in RestToXContentListener instead?
There was a problem hiding this comment.
Yea that was because we didn't have that check in the general listener. Should we maybe make that move in a separate PR? Even though it seems like "nothing could go wrong" by doing that, might be better having it in isolation?
There was a problem hiding this comment.
Sure, sounds like a good plan.
|
@DaveCTurner thanks for taking a look. ++ to the request in general :) (maybe note I'm currently looking into refactoring the x-content building logic around this a little anyway to deal with the crazy way we manage resources so that would fit in nicely with that line of effort anyway) |
|
@DaveCTurner all good here otherwise ? :) |
|
Yes, otherwise all good. Is fixing the |
|
Thanks @DaveCTurner fixed the listener now :) |
|
Thanks David! |
Same as elastic#72644. This is a much longer running action than normal get snapshots even so it should definitely be cancellable. Parallelization for this action will be introduced in a separate PR.
If this runs needlessly for large repositories (especially in timeout/retry situations) it's a significant memory+cpu hit => made it cancellable like we recently did for many other endpoints.
Same as #72644. This is a much longer running action than normal get snapshots even so it should definitely be cancellable. Parallelization for this action will be introduced in a separate PR.
Same as elastic#72644. This is a much longer running action than normal get snapshots even so it should definitely be cancellable. Parallelization for this action will be introduced in a separate PR.
If this runs needlessly for large repositories (especially in timeout/retry situations)
it's a significant memory+cpu hit => made it cancellable like we recently did for many
other endpoints.