Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support hierarchical task cancellation #54757

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Apr 3, 2020

With this change, when a task is canceled, the task manager will cancel not only its direct child tasks but all its descendant tasks.

Closes #50990

@dnhatn dnhatn added >enhancement :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. v8.0.0 v7.8.0 labels Apr 3, 2020
@dnhatn dnhatn requested a review from ywelsch April 3, 2020 21:11
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Task Management)

@dnhatn dnhatn requested a review from imotov April 3, 2020 21:12
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

holder.cancel(reason);
}
}
return cancellableTasks.values().stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to eventually optimize this, in case there might be a very large list of cancellable tasks. Also, this does not have proper happens-before, as iterating a concurrent map after an element has been added is not guaranteed to yield the element (it's eventually consistent).

@dnhatn
Copy link
Member Author

dnhatn commented Apr 6, 2020

I wonder if we need to eventually optimize this, in case there might be a very large list of cancellable tasks

++ This is on the plan. This optimization can be useful for CSS with minimize_round_trip disabled.

Also, this does not have proper happens-before, as iterating a concurrent map after an element has been added is not guaranteed to yield the element (it's eventually consistent).

I see your point. We don't need to cancel children of tasks that are registered after we placed the ban because they do not have any child (they are canceled and unregistered before starting). Anyway, this logic is quite subtle. I will look into this in a follow-up.

I will merge this PR to unblock Igor's work. Thank you for reviews, Yannick!

@dnhatn dnhatn merged commit 61e5350 into elastic:master Apr 6, 2020
@dnhatn dnhatn deleted the hierarchical-cancellation branch April 6, 2020 16:00
dnhatn added a commit that referenced this pull request Apr 13, 2020
dnhatn added a commit that referenced this pull request Apr 13, 2020
With this change, when a task is canceled, the task manager will cancel
not only its direct child tasks but all also its descendant tasks.

Closes #50990
jimczi added a commit to jimczi/elasticsearch that referenced this pull request Jun 18, 2020
This change allows the submit async search task to cancel children
and removes the manual indirection that cancels the search task when the submit
task is cancelled. This is now handled by the task cancellation, which can cancel
grand-children since elastic#54757.
jimczi added a commit that referenced this pull request Jun 24, 2020
)

This change allows the submit async search task to cancel children
and removes the manual indirection that cancels the search task when the submit
task is cancelled. This is now handled by the task cancellation, which can cancel
grand-children since #54757.
jimczi added a commit that referenced this pull request Jun 24, 2020
)

This change allows the submit async search task to cancel children
and removes the manual indirection that cancels the search task when the submit
task is cancelled. This is now handled by the task cancellation, which can cancel
grand-children since #54757.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. >enhancement v7.8.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cancelling a task's grandchildren when the task is cancelled
4 participants