Broadcast cancellation to only nodes have outstanding child tasks #54312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnhatn
added
>enhancement
:Distributed Coordination/Task Management
|
Pinging @elastic/es-distributed (:Distributed/Task Management) |
|
Please hold off the review. I am investigating some test failures that are related to this change. |
|
I wonder if, as part of this iteration, we should already wait with the unbanning until all child tasks have been completed (successfully or not). |
++. I will add it to this PR. |
|
This is ready for reviews :). |
ywelsch
reviewed
Mar 30, 2020
modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java
Outdated
Show resolved
Hide resolved
modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java
Outdated
Show resolved
Hide resolved
...ava/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java
Outdated
Show resolved
Hide resolved
...ava/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java
Outdated
Show resolved
Hide resolved
...ava/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/search/SearchCancellationIT.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java
Outdated
Show resolved
Hide resolved
ywelsch
approved these changes
Apr 1, 2020
client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksRequest.java
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/action/admin/cluster/node/tasks/cancel/CancelTasksRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestCancelTasksAction.java
Outdated
Show resolved
Hide resolved
|
Thanks Yannick. |
dnhatn
added a commit
that referenced
this pull request
Apr 1, 2020
The main task can fail if it is canceled while some of its child tasks are not forked yet. This commit relaxes the assertion for that situation. Relates #54312
dnhatn
added a commit
to dnhatn/elasticsearch
that referenced
this pull request
Apr 6, 2020
…astic#54312) Today when canceling a task we broadcast ban/unban requests to all nodes in the cluster. This strategy does not scale well for hierarchical cancellation. With this change, we will track outstanding child requests and broadcast the cancellation to only nodes that have outstanding child tasks. This change also prevents a parent task from sending child requests once it got canceled. Relates elastic#50990 Supersedes elastic#51157 Co-authored-by: Igor Motov <[email protected]> Co-authored-by: Yannick Welsch <[email protected]>
dnhatn
added a commit
that referenced
this pull request
Apr 6, 2020
dnhatn
added a commit
that referenced
this pull request
Apr 6, 2020
…4312) Today when canceling a task we broadcast ban/unban requests to all nodes in the cluster. This strategy does not scale well for hierarchical cancellation. With this change, we will track outstanding child requests and broadcast the cancellation to only nodes that have outstanding child tasks. This change also prevents a parent task from sending child requests once it got canceled. Relates #50990 Supersedes #51157 Co-authored-by: Igor Motov <[email protected]> Co-authored-by: Yannick Welsch <[email protected]>
dnhatn
added a commit
that referenced
this pull request
May 6, 2020
dnhatn
added a commit
to dnhatn/elasticsearch
that referenced
this pull request
May 6, 2020
We fail to unregister the child node in registerAndExecute if the parent task is being canceled. This leads to a bug where a cancel request never completes. Closes elastic#55875 Relates elastic#54312
dnhatn
added a commit
that referenced
this pull request
May 7, 2020
dnhatn
added a commit
that referenced
this pull request
May 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today when canceling a task we broadcast ban/unban requests to all nodes in the cluster. This strategy does not scale well for hierarchical cancellation. With this change, we will track outstanding child requests and broadcast cancellation to only nodes having outstanding child tasks. This change also strengthens the coordination during cancellation. A parent task will no longer be able to send child requests via transport service once it gets cancelled, which streamlines cancellation on the parent task by not requiring to manually check whether the task is cancelled during the execution.
Relates #50990
Supersedes #51157
Co-authored-by: Igor Motov [email protected]
Co-authored-by: Yannick Welsch [email protected]