-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Busy wait until all tasks are removed #95494
Conversation
There's a small delay between we release the main task and all the tasks actually get removed from the task manager, make sure we don't fail the test immediately, but busy wait a bit.
@elasticmachine update branch |
Pinging @elastic/es-distributed (Team:Distributed) |
Do we know why this started failing? |
I wonder if this is also related to #94987 (comment). |
Yeah, it's definitely related to #94987 (comment) |
There have been a few task related test failures due to #94865 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM makes sense with the change to when we remove a task.
try { | ||
assertBusy(() -> assertEquals(0, node.transportService.getTaskManager().getTasks().size())); | ||
} catch (Exception e) { | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Maybe just add throws Exception
to this method and all its callers? :)
Due to elastic#94865, we now send an ack before the task gets removed, so we can see a non-zero amount of tasks in the task manager for a short amount of time. We can just busy wait until the task gets removed. See elastic#95494, elastic#94987
There's a small delay between we release the main task and all the tasks actually get removed from the task manager, make sure we don't fail the test immediately, but busy wait a bit.
Fixes #95425