Fix flaky SearchCancellationIT tests to avoid race condition #5656
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
Signed-off-by: Daniel Widdis <widdis@gmail.com>
dbwiddis
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
andrross
reviewed
Dec 29, 2022
| @@ -241,7 +241,8 @@ public void testCancellationDuringQueryPhaseUsingRequestParameter() throws Excep | |||
| .execute(); | |||
| awaitForBlock(plugins); | |||
| // sleep for cancellation timeout to ensure scheduled cancellation task is actually executed | |||
There was a problem hiding this comment.
This comment is no longer accurate. How about a static sleepForTimeout() or similar method so you only have to write the comment once?
Also, any idea why 2 seconds is used everywhere here? Would be great to make that something shorter (like 100ms) to speed up the test.
There was a problem hiding this comment.
But I was having so much fun copying and pasting with The Key. Sure, I'll make a static call. Not sure what the best minimum should be, but I'm guessing I could make it 1 second at least?
There was a problem hiding this comment.
Sure. You'll make it twice as fast!
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5656 +/- ##
============================================
+ Coverage 70.96% 71.10% +0.14%
- Complexity 58550 58648 +98
============================================
Files 4760 4760
Lines 279515 279515
Branches 40348 40348
============================================
+ Hits 198367 198762 +395
+ Misses 65037 64576 -461
- Partials 16111 16177 +66
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
andrross
approved these changes
Dec 29, 2022
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Dec 29, 2022
* Add waiting time to account for Thread.sleep inaccuracy Signed-off-by: Daniel Widdis <widdis@gmail.com> (cherry picked from commit d248643) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
andrross
pushed a commit
that referenced
this pull request
Dec 29, 2022
…5657) * Add waiting time to account for Thread.sleep inaccuracy Signed-off-by: Daniel Widdis <widdis@gmail.com> (cherry picked from commit d248643) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kotwanikunal
pushed a commit
that referenced
this pull request
Jan 25, 2023
…5657) * Add waiting time to account for Thread.sleep inaccuracy Signed-off-by: Daniel Widdis <widdis@gmail.com> (cherry picked from commit d248643) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
Signed-off-by: Daniel Widdis widdis@gmail.com
Description
Thread.sleep()does not sleep for precise amounts:Tests in the
SearchCancellationITclass relied on sleeping for exactly the cancellation timeout, which creates potential race conditions and occasional test failures.This PR adds an extra 100ms of sleep time before checking the cancellation status.
Issues Resolved
Fixes #2242
Fixes #2311
Fixes #2763
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.