-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix [BUG] ClusterMaxMergesAtOnceIT is flaky #19647
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Lamine Idjeraoui <[email protected]>
update change log Signed-off-by: Lamine Idjeraoui <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #19647 +/- ##
============================================
- Coverage 73.09% 73.03% -0.06%
+ Complexity 70723 70694 -29
============================================
Files 5725 5725
Lines 323796 323796
Branches 46886 46886
============================================
- Hits 236673 236493 -180
- Misses 68009 68241 +232
+ Partials 19114 19062 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @andrross |
assertEquals(20, ((OpenSearchTieredMergePolicy) indexService.getIndexSettings().getMergePolicy(true)).getMaxMergeAtOnce()); | ||
assertEquals(20, ((OpenSearchTieredMergePolicy) secondIndexService.getIndexSettings().getMergePolicy(true)).getMaxMergeAtOnce()); | ||
|
||
int replicas = randomIntBetween(1, Math.max(1, internalCluster().numDataNodes() - 1)); |
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.
More for my own curiosity than anything -- how does this end up fixing the test?
By the default, the replica count would be 1, based on the implementation of indexSettings()
on line 56. So I don't understand how setting the replica count to a random number between 1 and 1 (since it looks like numDataNodes = 2
) changes anything.
I'm not particularly familiar with these tests, but I'm trying to understand more. Thanks!
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.
Maybe generating a random integer here is enough to change the random result from randomFrom(dataNodes)
on line 108 (before)/111 (after)?
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.
@msfroh I think you're right. I confirmed that just adding a randomInt()
call here and doing nothing with the result is enough to make the test pass.
Resolves #18056
[BUG] ClusterMaxMergesAtOnceIT is flaky
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.