Reindex rethrottle persistent task #51599
Conversation
This adds support for rethrottling resilient/persistent reindex through updating the .reindex index and notifying the task. This ensures that the new throttle value sticks on failovers while also ensuring that the task wakes up immediately if it had a very low throttle value. Related to elastic#42612
henningandersen
added
>non-issue
:Distributed Indexing/Reindex
|
Pinging @elastic/es-distributed (:Distributed/Reindex) |
There was a problem hiding this comment.
I think this has a bug currently.
BulkByScrollParallelizationHelper#sendSubRequests will use the requests per second parameter in request.forSlice which means it will use the original value (not updated) on a failover.
I think you are attempting to avoid mutating the request inside the index (and storing an additional request per second field) to retain the original request? Is that of value? We could store like a rethrottle audit field and mutate the request. So there is like a list field showing the submitted throttle values (and maybe time) and update the request to whatever the current value is.
| private final ActionListener<ReindexTaskStateDoc> finishedListener; | ||
| private final Runnable onCheckpointAssignmentConflict; | ||
| private Consumer<Float> requestsPerSecondObserver; |
There was a problem hiding this comment.
Do we need this? I feel like adding the "poll" interaction model when we are doing all the working to implement the "push" component makes the interaction with the state updater and task more complicated that required.
There was a problem hiding this comment.
Thanks, I agree, removed in b6096a9
|
You could also mutate the request immediately when putting it out of the index and still have a dedicated field for the requests per second. |
Modify reindex request instead of passing in requestsPerSecond Remove rethrottle on .reindex version conflict.
That was kind of on purpose, since slicing is not supported for resilient reindex anyway. I changed it to modify the request instead, that is a bit nicer. |
…_rethrottle_via_index
…_rethrottle_via_index
…_rethrottle_via_index
|
Test failure reported here #51926, so: |
This adds support for rethrottling resilient/persistent reindex
through updating the .reindex index and notifying the task. This
ensures that the new throttle value sticks on failovers while
also ensuring that the task wakes up immediately if it had a very
low throttle value.
Related to #42612
This is the replacement for #50854.
Tests on the transport layer are missing, but I wanted to ensure
we agree on approach first and then tackle adding those
in a follow-up.