Fix CancellableTasksIT.testChildrenTasksCancelledOnTimeout() #129513
Closed
Conversation
This file contains hidden or 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
A race condition exists in TransportTestAction that could result in a subrequest timing out before the corresponding task has been registered on the remote node. This change add a trace log message to the task manager for when a children list is not found in cancelChildLocal() and a counter that is incremented. The test still verifies that at least one child task is cancelled via the expected cancellation mechanism, manually cancelling any orphaned tasks and verifies that the number of manual cancellations required equals the number of cancelChildLocal() calls observed where a children list was not found. Closes elastic#123568
JeremyDahlgren
added
>test
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
I am not sure whether we want to fix it as a pure test problem. I'd personally rather keep it and fix the underlying issue. If we do prefer to fix the test for now, I think it would be better to fix the test assumption by ensuring a cancellation request is sent after the associated task is executing, i.e. some coordinations between sending |
|
Yeah I'm inclined to agree. Seems weird to make this test bend itself around the bugs in the very subsystem it's supposed to be testing. |
|
Closing per the review comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
A race condition exists in
TransportTestActionthat could result in a subrequest timing out before the corresponding task has been registered on the remote node. This change add a trace log message to theTaskManagerfor when a children list is not found incancelChildLocal()and a counter that is incremented. The test still verifies that at least one child task is cancelled via the expected cancellation mechanism, manually cancelling any orphaned tasks and verifies that the number of manual cancellations required equals the number ofcancelChildLocal()calls observed where a children list was not found.To reproduce the failure reliably I changed the timeout in
TransportTestActionfrom 400 milliseconds to 1 milliseconds when testing locally.See #123568 for details about possible follow ups on either implementing cancellation retry support, refactoring
TransportTestActionto enforce ordering of requests so timeouts do not occur until the corresponding remote task has been registered, and discussion of support for transport-level timeouts in general.Closes #123568