Skip to content

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Dec 17, 2020

This BWC is no longer needed after backporting #66066 to 7.x.

Closes #66518

@dnhatn dnhatn added >non-issue :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. v8.0.0 labels Dec 17, 2020
@dnhatn dnhatn requested a review from DaveCTurner December 17, 2020 14:44
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Dec 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

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.

I think we can fix the test rather than simply removing it :)

if (event.nodesRemoved()) {
synchronized (bannedParents) {
lastDiscoveryNodes = event.state().getNodes();
// Remove all bans that were registered by nodes that are no longer in the cluster state
Copy link
Contributor

Choose a reason for hiding this comment

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

So good 🎉

}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/66518")
public void testTaskCancellationOnCoordinatingNodeLeavingTheCluster() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this test seems somewhat useful, although I see that it's not quite valid any more. Can we make it valid again by, say, closing the connections from the "removed" nodes?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1, I fixed it in 5b57123

Copy link
Contributor

Choose a reason for hiding this comment

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

The adjusted test fails for me after a few iterations. Not reproducibly, otherwise I'd share a seed, but hopefully you can see this too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for checking. I will run it a few hundred iterations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed the fix in ff8b89c

@dnhatn dnhatn requested a review from DaveCTurner December 17, 2020 16:50
@dnhatn
Copy link
Member Author

dnhatn commented Dec 18, 2020

@DaveCTurner Would you please take another look? Thank you.

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.

LGTM

@dnhatn
Copy link
Member Author

dnhatn commented Dec 18, 2020

Thanks David.

@dnhatn dnhatn merged commit 67bf9fd into elastic:master Dec 18, 2020
@dnhatn dnhatn deleted the remove-bwc-task-ban branch December 18, 2020 14:24
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. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: testTaskCancellationOnCoordinatingNodeLeavingTheCluster fails

4 participants