-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Fixing rollup state tests after onFailure ordering change #45784
Fixing rollup state tests after onFailure ordering change #45784
Conversation
Pinging @elastic/es-analytics-geo |
I ran locally with this change 10k+ times and it never failed. Without this change I would get periodic failures (matching the closed issue). |
@@ -946,10 +956,11 @@ protected void doNextBulk(BulkRequest request, ActionListener<BulkResponse> next | |||
assertTrue(indexer.maybeTriggerAsyncJob(System.currentTimeMillis())); | |||
assertThat(indexer.getState(), equalTo(IndexerState.INDEXING)); | |||
latch.countDown(); | |||
assertBusy(() -> assertTrue(isFinished.get())); | |||
ESTestCase.awaitBusy(() -> isFinished.get()); |
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.
nitpick: I think this awaitBusy can be removed. If the assertBusy above passes, this will always pass (unless there is some async condition that can change back to false). If the assertBusy fails if will fail the test and this line wont execute.
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.
You are correct! Let me fix it up really quick.
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.
Changes to the flow of state LGTM :)
) After the PR elastic#45676 onFailure is now called before the indexer state has transitioned out of indexing. To fix these tests, I added a new check to make sure that we don't mark it as failed until AFTER doSaveState is called with a STARTED indexer.
After the PR #45676
onFailure
is now called before the indexer state has transitioned out of indexing.To fix these tests, I added a new check to make sure that we don't mark it as failed until AFTER doSaveState is called with a
STARTED
indexer.Sorry for missing these test changes in the earlier PR.
closes #45770