Cancel task and descendants on channel disconnects #56620

dnhatn · 2020-05-12T17:19:38Z

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

elasticmachine · 2020-05-12T17:19:40Z

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

DaveCTurner

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

...ava/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java

dnhatn · 2020-05-12T20:57:35Z

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

DaveCTurner

Thanks @dnhatn. A couple more questions...

server/src/main/java/org/elasticsearch/tasks/TaskManager.java

jimczi

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

dnhatn · 2020-05-13T02:38:00Z

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

...ava/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java

jimczi

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

server/src/main/java/org/elasticsearch/tasks/TaskManager.java

server/src/main/java/org/elasticsearch/node/Node.java

This reverts commit 956859c.

dnhatn · 2020-05-13T23:06:03Z

@DaveCTurner @jimczi Thank you for you reviews.

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

Like #56620, this change relies on channel disconnect instead of node leave events to remove parent-task ban markers. Relates #65443 Relates #56620

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

Like #56620, this change relies on channel disconnect instead of node leave events to remove parent-task ban markers. Relates #65443 Relates #56620

Cancel task and descendants on channel disconnects

59f50a0

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

dnhatn requested review from DaveCTurner, Tim-Brooks and javanna May 12, 2020 17:19

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 12, 2020

dnhatn added 2 commits May 12, 2020 14:08

Merge branch 'master' into cancel-on-disconnect

d92ef10

fix test

22b28fa

DaveCTurner reviewed May 12, 2020

View reviewed changes

dnhatn added 4 commits May 12, 2020 16:38

Merge branch 'master' into cancel-on-disconnect

92bbcf2

warning log

d4f7586

remove tracker on close

cbe02c7

assertion

09a612e

dnhatn requested a review from DaveCTurner May 12, 2020 20:57

wording

3b4e3be

DaveCTurner reviewed May 12, 2020

View reviewed changes

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 reviewed May 12, 2020

View reviewed changes

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 added 3 commits May 12, 2020 20:16

single listener

1d2de32

track and close concurrently

bdfa4f2

simulate other condition

8ae7042

dnhatn requested review from DaveCTurner and jimczi May 13, 2020 02:38

dnhatn added 2 commits May 12, 2020 22:47

Merge branch 'master' into cancel-on-disconnect

ebcd4fa

stylecheck

37a77d6

jimczi reviewed May 13, 2020

View reviewed changes

...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

dnhatn added 2 commits May 13, 2020 11:30

Move cancellation descendants to cancellation service

bd29504

Merge branch 'master' into cancel-on-disconnect

8e78fad

dnhatn requested a review from jimczi May 13, 2020 15:37

jimczi approved these changes May 13, 2020

View reviewed changes

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 added 3 commits May 13, 2020 17:08

Merge branch 'master' into cancel-on-disconnect

9329a10

assign earlier

956859c

Revert "assign earlier"

fd94b97

This reverts commit 956859c.

dnhatn merged commit 595ce8b into elastic:master May 13, 2020

dnhatn deleted the cancel-on-disconnect branch May 13, 2020 23:06

dnhatn added the backport pending label May 13, 2020

dnhatn removed the backport pending label May 14, 2020

martijnvg mentioned this pull request May 14, 2020

[CI] TaskManagerTests#testTrackingChannelTask #56746

Closed

dnhatn mentioned this pull request May 27, 2020

Support task cancellation cross clusters #55779

Closed

dnhatn mentioned this pull request Nov 24, 2020

Send ban parent per outstanding child connection #65443

Merged

dnhatn mentioned this pull request Dec 8, 2020

Remove parent-task bans on channels disconnect #66066

Merged

dnhatn added a commit that referenced this pull request Dec 16, 2020

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 mentioned this pull request Dec 16, 2020

Remove parent-task bans on channels disconnect #66480

Merged

dnhatn added a commit that referenced this pull request Dec 17, 2020

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

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

DaveCTurner mentioned this pull request Dec 22, 2022

Task cancellation should use same TCP channel as request to be cancelled #92532

Open

DaveCTurner mentioned this pull request Apr 7, 2023

Improve resilience to network disconnects #95100

Open

Cancel task and descendants on channel disconnects #56620

Cancel task and descendants on channel disconnects #56620

Uh oh!

Conversation

dnhatn commented May 12, 2020

Uh oh!

elasticmachine commented May 12, 2020

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnhatn commented May 12, 2020

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnhatn commented May 13, 2020

Uh oh!

Uh oh!

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnhatn commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants