Always enforce a default tier preference (ENFORCE_DEFAULT_TIER_PREFERENCE must always be true) #79723
Conversation
It's just always true, these tests need to learn to live with that.
The tests that checked what happens when we don't enforce a tier preference don't need to exist anymore. Likewise, since we always enforce we can rename the tests that check what happens when we do.
joegallo
added
>breaking
:Data Management/ILM+SLM
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
We'll be removing it one day, and it's only allowed to be true anyway, so there's no point in having it.
|
If we end up going with this version of things, then a follow up PR could short cut a bunch of if/elses, since only the true side of those if/elses would ever get executed. Alternatively, we could skip this PR and only go with that other PR -- this setting hangs around with a true or false value, and we don't care what it is (or even that it exists), because the behavior doesn't check the value of the setting anymore at all. I talked this approach over with @dakrone and we liked it enough that I'm going to write that up now. I'll leave this PR up though just in case we decide to go this way instead of that way. edit: that'd be #79751 |
| new Setting.Validator<>() { | ||
| @Override | ||
| public void validate(Boolean value) { | ||
| if (value == Boolean.FALSE) { |
There was a problem hiding this comment.
I think I would prefer:
| if (value == Boolean.FALSE) { | |
| if (value != Boolean.TRUE) { |
then there is no doubt about null being illegal too.
There was a problem hiding this comment.
b5e6a5d, sure thing.
| // if we inject a default tier preference, then this test wouldn't be valid anymore, | ||
| // so let's turn that off | ||
| enforceDefaultTierPreference(false); | ||
|
|
There was a problem hiding this comment.
I think this invalidates the purpose of the test. And on 8.0 we may still see indices with no tier preference (since they could have been created in 7.x). So ideally we would allow the test to create data with no tier preference somehow.
I would be Ok to defer that test to a follow-up though.
There was a problem hiding this comment.
72dee11 should handle these -- I've gone back to the original intent of the tests by manually setting the tier preference back to null.
|
|
||
| // we can't have the pre-migration indices getting tier preferences auto-assigned, | ||
| // if we did, then we wouldn't really be testing the migration *to* data tiers anymore :D | ||
| enforceDefaultTierPreference(false); |
There was a problem hiding this comment.
This is another case where we would ideally add such indices with no tier preference (as if they were created in 7.x) and see the migration cure them.
| assertThat(indexSettings.get(DataTier.TIER_PREFERENCE), is("data_warm,data_hot")); | ||
| assertThat(indexSettings.get(DataTier.TIER_PREFERENCE), is(DataTier.DATA_CONTENT)); |
There was a problem hiding this comment.
I think this is an essential part of the migration that is no longer tested in 8.0? I think we should ensure that we can add indices with no tier preference for test purpose and check that the migration fills them in.
by setting the tier preference back to null after the index has been created
|
Closing this one in favor of #79751 |
Part of #76147, follow up to #79210 and #79275
#79210 added this new setting, and #79275 made it so the value defaults to
true. This PR goes one further, and makes it so that the setting is only ever allowed to betrue. Mostly it's adjusting the tests to account for that, there were some tests where we were still explicitly setting the value tofalsein order to test 'what if' scenarios.