-
Notifications
You must be signed in to change notification settings - Fork 1.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
Fix Flaky - testForceMergeWithSoftDeletesRetentionAndRecoverySource #5364 #11494
Fix Flaky - testForceMergeWithSoftDeletesRetentionAndRecoverySource #5364 #11494
Conversation
Compatibility status:Checks if related components are compatible with change 83938fb Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/performance-analyzer.git] |
Signed-off-by: Sarthak Aggarwal <[email protected]>
f68b0f1
to
f3f1c76
Compare
Signed-off-by: Sarthak Aggarwal <[email protected]>
❌ Gradle check result for f68b0f1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11494 +/- ##
============================================
+ Coverage 71.37% 71.41% +0.04%
- Complexity 59113 59143 +30
============================================
Files 4893 4903 +10
Lines 277831 277983 +152
Branches 40367 40382 +15
============================================
+ Hits 198288 198518 +230
+ Misses 63042 62969 -73
+ Partials 16501 16496 -5 ☔ View full report in Codecov by Sentry. |
❕ Gradle check result for 83938fb: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
This PR is stalled because it has been open for 30 days with no activity. |
This PR is stalled because it has been open for 30 days with no activity. |
This PR is stalled because it has been open for 30 days with no activity. |
@@ -1822,8 +1822,8 @@ public void testForceMergeWithSoftDeletesRetentionAndRecoverySource() throws Exc | |||
) | |||
) { | |||
int numDocs = scaledRandomIntBetween(10, 100); | |||
boolean useRecoverySource = randomBoolean() || omitSourceAllTheTime; |
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.
Move the useRecoverySource
flag out of the for loop makes every document has same flag, the randomness is lost, I think we need to keep the randomness in order to cover different cases.
ParsedDocument doc = testParsedDocument(Integer.toString(i), null, testDocument(), B_1, null, useRecoverySource); | ||
if (randomBoolean()) { | ||
boolean isDeleted = randomBoolean(); | ||
if (isDeleted) { |
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.
Does this mean that if isDeleted
is true, for each document, we delete it firstly, and then add them back? If so the randomness is also lost.
@@ -1848,9 +1848,8 @@ public void testForceMergeWithSoftDeletesRetentionAndRecoverySource() throws Exc | |||
liveDocsWithSource.remove(doc.id()); | |||
} | |||
} | |||
if (randomBoolean()) { |
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.
Why remove the if condition?
@@ -1896,7 +1895,6 @@ public void testForceMergeWithSoftDeletesRetentionAndRecoverySource() throws Exc | |||
numSegments = searcher.getDirectoryReader().leaves().size(); | |||
} | |||
if (numSegments == 1) { | |||
boolean useRecoverySource = randomBoolean() || omitSourceAllTheTime; |
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.
If numSegments = 1
, we add a new document here, so the flag should be generated randomly.
Description
The document that is deleted is the one that would be indexed again while we check for the
useRecoverySource
flag to populate theliveDocsWithSource
setThe tests are also passing with the failed seeds
C75D09DC5A0C3458
1766E6A3B733A7AA
10K Iterations Passed
Related Issues
Resolves #5364
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.