Fix flaky test by preventing duplicate keys in random mapping fields #9184
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
dbwiddis
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah,
sachinpkale and
sohami
as code owners
|
Compatibility status: |
Gradle Check (Jenkins) Run Completed with:
|
|
Annoying when one flaky test fix has a failure from another flaky test. :) #9178 |
Gradle Check (Jenkins) Run Completed with:
|
|
Compatibility status: |
|
Latest flaky test failure tracked in #1703... |
dbwiddis
commented
Aug 11, 2023
There was a problem hiding this comment.
@Poojita-Raj I don't think the suggested changes are needed in this case. The collisions are extremely rare and the occasional decrement is still well within (and barely impacts) the random ranges being generated.
If you really think this needs to be engineered more robustly, I'd suggest adding another utility method in OpenSearchTestCase similar to generateRandomStringArray() that creates unique strings, and then audit all the tests to see if there are other cases like this one.
For flaky tests, could we also mention how many times the test was run to ensure it's no longer flaky for confidence before we merge it in?
I originally closed the flaky test issue because I could calculate the extremely rare probability of a failure, and asked a similar question here: #6739 (comment)
This was the only failure of this test that I could find in the last 2500 runs using the script, and a search of issues on this repo indicates it was the only case reported ever.
Happy to run this N times to prove no reproduction, but nobody has defined what N should be. Given it's only failed once in 12500 test runs and the probability is at least 1 in 20,000 or so (probably more) I think I'd have to test it at least 30,000 times to make sure it's "fixed". I don't think that's necessary but am willing to write a script to do so.
...t-high-level/src/test/java/org/opensearch/client/indices/GetIndexTemplatesResponseTests.java
Outdated
Show resolved
Hide resolved
test/framework/src/main/java/org/opensearch/index/RandomCreateIndexGenerator.java
Show resolved
Hide resolved
dbwiddis
commented
Aug 12, 2023
There was a problem hiding this comment.
@Poojita-Raj looked at it again and decided to just replace for loops with while checking size, to preserve original size. Realized the aliases were already unique because the index was used as part of the name, so backed out that change.
Should be GTG now. I don't think it's necessary to try to reproduce this again as the cause is well understood, the reproducing duplicate keys still occur with the random seed in the linked issue, and the test doesn't fail.
test/framework/src/main/java/org/opensearch/index/RandomCreateIndexGenerator.java
Show resolved
Hide resolved
Gradle Check (Jenkins) Run Completed with:
|
|
Compatibility status: |
Gradle Check (Jenkins) Run Completed with:
|
|
Test failure may have been fixed by #9079 Will rebase |
|
Compatibility status: |
Gradle Check (Jenkins) Run Completed with:
|
|
Both |
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
|
Compatibility status: |
Gradle Check (Jenkins) Run Completed with:
|
|
This has fallen off the "front page" of PR reviews, so bumping for attention. Minor fix, shouldn't be controversial. @Poojita-Raj (answered your earlier review), @mch2 @andrross @saratvemulapalli @dblock @ryanbogan @owaiskazi19 :-) |
andrross
approved these changes
Aug 17, 2023
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Aug 17, 2023
…9184) * Prevent duplicate keys in random mapping fields Signed-off-by: Daniel Widdis <widdis@gmail.com> * Aliases were already unique. Used while loop for indices to keep size Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> (cherry picked from commit e1c40b4) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dbwiddis
pushed a commit
that referenced
this pull request
Aug 17, 2023
…9184) (#9406) * Prevent duplicate keys in random mapping fields * Aliases were already unique. Used while loop for indices to keep size --------- (cherry picked from commit e1c40b4) 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>
austintlee
pushed a commit
to austintlee/OpenSearch
that referenced
this pull request
Aug 25, 2023
…pensearch-project#9184) * Prevent duplicate keys in random mapping fields Signed-off-by: Daniel Widdis <widdis@gmail.com> * Aliases were already unique. Used while loop for indices to keep size Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com>
kkmr
pushed a commit
to kkmr/OpenSearch
that referenced
this pull request
Aug 28, 2023
…pensearch-project#9184) * Prevent duplicate keys in random mapping fields Signed-off-by: Daniel Widdis <widdis@gmail.com> * Aliases were already unique. Used while loop for indices to keep size Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Kiran Reddy <kkreddy@amazon.com>
kaushalmahi12
pushed a commit
to kaushalmahi12/OpenSearch
that referenced
this pull request
Sep 12, 2023
…pensearch-project#9184) * Prevent duplicate keys in random mapping fields Signed-off-by: Daniel Widdis <widdis@gmail.com> * Aliases were already unique. Used while loop for indices to keep size Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>
brusic
pushed a commit
to brusic/OpenSearch
that referenced
this pull request
Sep 25, 2023
…pensearch-project#9184) * Prevent duplicate keys in random mapping fields Signed-off-by: Daniel Widdis <widdis@gmail.com> * Aliases were already unique. Used while loop for indices to keep size Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Ivan Brusic <ivan.brusic@flocksafety.com>
shiv0408
pushed a commit
to Gaurav614/OpenSearch
that referenced
this pull request
Apr 25, 2024
…pensearch-project#9184) * Prevent duplicate keys in random mapping fields Signed-off-by: Daniel Widdis <widdis@gmail.com> * Aliases were already unique. Used while loop for indices to keep size Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Shivansh Arora <hishiv@amazon.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.
Description
Although rare, if you throw random alpha strings together often enough, you'll get a collision.
Related Issues
Fixes #6739
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.