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
Reset TieredMP settings only if the value actually changed #9497
Conversation
LGTM |
I wonder if this is the cause of #8890 |
yes indeed.... I will fix the other providers too and add tests |
TieredMergePolicyProvider mp = new TieredMergePolicyProvider(createStore(EMPTY_SETTINGS), service); | ||
assertThat(mp.getMergePolicy().getNoCFSRatio(), equalTo(0.1)); | ||
|
||
assertEquals(mp.getMergePolicy().getForceMergeDeletesPctAllowed(), TieredMergePolicyProvider.DEFAULT_EXPUNGE_DELETES_ALLOWED, 0.0d); |
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.
I think we need to add a check that other settings do not spring back to their defaults, which I believe was the original issue (update one setting and have all the rest go back to default).
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.
see the bottom of the test there is a update call with empty settings
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.
missed that. All good then. Sorry for the noise.
Fix LGTM. Added one comment about the test. |
Thanks for picking it up! 👍 |
@mikemccand @bleskes I fixed the other MPs as well can you take a look? |
Due to some unreleased refactorings we lost the persitence of a perviously set values in MergePolicyProvider. This commit adds this back and adds a simple unittest. Closes elastic#8890
Due to some unreleased refactorings we lost the persitence of
a perviously set values in TieredMPProvider. This commit adds this
back and adds a simple unittest.
Closes #8890