AsyncOperator#isFinished must never return true on failure #104029
Merged
Conversation
This file contains 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
|
Hi @dnhatn, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
nik9000
approved these changes
Jan 8, 2024
|
@ChrisHegarty @nik9000 Thanks for reviewing. |
dnhatn
added a commit
to dnhatn/elasticsearch
that referenced
this pull request
Jan 8, 2024
…04029) Enrich IT tests can return OK with some missing results instead of Failure when the enrich lookup hits circuit breakers. This is due to a race condition in isFinished and onFailure within the AsyncOperator. When an async lookup fails, we set the exception and then discard pages. Unfortunately, in the isFinished method, we perform the checks in the same order: first, we check for failure, and then we check for outstanding pages. If there is a long pause between these steps, isFinished might not detect the failure but see no outstanding pages, leading it to return true despite the presence of a failure. This change swaps the order of the checks.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jan 8, 2024
…104070) Enrich IT tests can return OK with some missing results instead of Failure when the enrich lookup hits circuit breakers. This is due to a race condition in isFinished and onFailure within the AsyncOperator. When an async lookup fails, we set the exception and then discard pages. Unfortunately, in the isFinished method, we perform the checks in the same order: first, we check for failure, and then we check for outstanding pages. If there is a long pause between these steps, isFinished might not detect the failure but see no outstanding pages, leading it to return true despite the presence of a failure. This change swaps the order of the checks.
dnhatn
added a commit
to dnhatn/elasticsearch
that referenced
this pull request
Jan 8, 2024
…04029) Enrich IT tests can return OK with some missing results instead of Failure when the enrich lookup hits circuit breakers. This is due to a race condition in isFinished and onFailure within the AsyncOperator. When an async lookup fails, we set the exception and then discard pages. Unfortunately, in the isFinished method, we perform the checks in the same order: first, we check for failure, and then we check for outstanding pages. If there is a long pause between these steps, isFinished might not detect the failure but see no outstanding pages, leading it to return true despite the presence of a failure. This change swaps the order of the checks.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jan 8, 2024
…104079) Enrich IT tests can return OK with some missing results instead of Failure when the enrich lookup hits circuit breakers. This is due to a race condition in isFinished and onFailure within the AsyncOperator. When an async lookup fails, we set the exception and then discard pages. Unfortunately, in the isFinished method, we perform the checks in the same order: first, we check for failure, and then we check for outstanding pages. If there is a long pause between these steps, isFinished might not detect the failure but see no outstanding pages, leading it to return true despite the presence of a failure. This change swaps the order of the checks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I have several CI instances running ESQL tests over the past week, and they have identified cases where enrich IT tests return
OKwith some missing results instead ofFailurewhen the enrich lookup hits circuit breakers. This is due to a race condition inisFinishedandonFailurewithin the AsyncOperator. When an async lookup fails, we set the exception and then discard pages. Unfortunately, in theisFinishedmethod, we perform the checks in the same order: first, we check for failure, and then we check for outstanding pages. If there is a long pause between these steps,isFinishedmight not detect the failure but see no outstanding pages, leading it to return true despite the presence of a failure. This change swaps the order of the checks.