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

Jump to bottom

Cancel task and descendants on channel disconnects #56620

Merged
merged 18 commits into from May 13, 2020
Merged

Cancel task and descendants on channel disconnects #56620

merged 18 commits into from May 13, 2020

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented May 12, 2020

If a channel gets disconnected, then we should cancel the tasks associated with that channel as their results won't be retrieved.

I opened this PR to provide more background for the discussion in #56327.

Closes #56327
Relates #56619

@dnhatn dnhatn added >enhancement :Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. v8.0.0 v7.9.0 labels May 12, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label May 12, 2020
DaveCTurner
DaveCTurner reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Nice work, much simpler than I expected :)

I think there's a minor leak here, and I left a few other small comments too.

server/src/main/java/org/elasticsearch/tasks/TaskManager.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/tasks/TaskManager.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/tasks/TaskManager.java Show resolved Hide resolved
...ava/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java Outdated Show resolved Hide resolved
@dnhatn
Copy link
Member Author

dnhatn commented May 12, 2020

Thanks @DaveCTurner for the helpful reviews. I've addressed your comments.

@dnhatn dnhatn requested a review from DaveCTurner May 12, 2020 20:57
DaveCTurner
DaveCTurner reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Thanks @dnhatn. A couple more questions...

server/src/main/java/org/elasticsearch/tasks/TaskManager.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/tasks/TaskManager.java Show resolved Hide resolved
jimczi
jimczi reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left some comments but the change looks great !
Relying on channel closing seems more reliable than cluster state update so I expect that cancellation will be much more reactive with this change.

server/src/main/java/org/elasticsearch/tasks/TaskManager.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/tasks/TaskManager.java Outdated Show resolved Hide resolved
@dnhatn
Copy link
Member Author

dnhatn commented May 13, 2020

@DaveCTurner @jimczi Thanks for reviews. It's ready again.

@dnhatn dnhatn requested review from DaveCTurner and jimczi May 13, 2020 02:38
@dnhatn dnhatn requested a review from jimczi May 13, 2020 15:37
jimczi
jimczi approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

Thanks for moving the code. I left one question, LGTM otherwise.

server/src/main/java/org/elasticsearch/tasks/TaskManager.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/node/Node.java Show resolved Hide resolved
@dnhatn
Copy link
Member Author

dnhatn commented May 13, 2020

@DaveCTurner @jimczi Thank you for you reviews.

@dnhatn dnhatn merged commit 595ce8b into elastic:master May 13, 2020
@dnhatn dnhatn deleted the cancel-on-disconnect branch May 13, 2020 23:06
dnhatn added a commit that referenced this pull request May 14, 2020
@dnhatn
Cancel task and descendants on channel disconnects (#56620)
566b23c 
If a channel gets disconnected, then we should cancel the tasks
associated with that channel as their results won't be retrieved.

Closes #56327
Relates #56619

Backport of #56620
@martijnvg martijnvg mentioned this pull request May 14, 2020
Closed
@dnhatn dnhatn mentioned this pull request May 27, 2020
Closed
@dnhatn dnhatn mentioned this pull request Nov 24, 2020
Merged
@dnhatn dnhatn mentioned this pull request Dec 8, 2020
Merged
dnhatn added a commit that referenced this pull request Dec 16, 2020
@dnhatn
Remove parent-task bans on channels disconnect (#66066)
0a57458 
Like #56620, this change relies on channel disconnect instead of node 
leave events to remove parent-task ban markers.

Relates #65443
Relates #56620
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Dec 16, 2020
@dnhatn
Remove parent-task bans on channels disconnect (elastic#66066)
e55f5d3 
Like elastic#56620, this change relies on channel disconnect instead of node
leave events to remove parent-task ban markers.

Relates elastic#65443
Relates elastic#56620
@dnhatn dnhatn mentioned this pull request Dec 16, 2020
Merged
dnhatn added a commit that referenced this pull request Dec 17, 2020
@dnhatn
Remove parent-task bans on channels disconnect (#66066)
59422f3 
Like #56620, this change relies on channel disconnect instead of node
leave events to remove parent-task ban markers.

Relates #65443
Relates #56620
alyokaz pushed a commit to alyokaz/elasticsearch that referenced this pull request Mar 10, 2021
@dnhatn
Remove parent-task bans on channels disconnect (elastic#66066)
e004e4a 
Like elastic#56620, this change relies on channel disconnect instead of node 
leave events to remove parent-task ban markers.

Relates elastic#65443
Relates elastic#56620
@DaveCTurner DaveCTurner mentioned this pull request Dec 22, 2022
Open
@DaveCTurner DaveCTurner mentioned this pull request Apr 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimczi jimczi jimczi approved these changes

@javanna javanna Awaiting requested review from javanna

@Tim-Brooks Tim-Brooks Awaiting requested review from Tim-Brooks

@DaveCTurner DaveCTurner Awaiting requested review from DaveCTurner

Assignees
No one assigned
Labels
:Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. >enhancement Team:Distributed Meta label for distributed team v7.9.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cancel transport request handling on disconnection
5 participants
@dnhatn @elasticmachine @DaveCTurner @jimczi @jakelandis