Move DataTier.TIER_PREFERENCE_SETTING registration out of XPackPlugin #78995
Conversation
As a follow up to DataTier having been moved to server/
joegallo
added
:Distributed Coordination/Allocation
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
LGTM, and I can't think of a reason why this wouldn't be ok to do. Maybe @henningandersen could take 2 min to confirm that though ? :) Thanks!
| @@ -444,7 +445,8 @@ private static ShardFollowTask createShardFollowTask( | |||
| MergeSchedulerConfig.AUTO_THROTTLE_SETTING, | |||
| MergeSchedulerConfig.MAX_MERGE_COUNT_SETTING, | |||
| MergeSchedulerConfig.MAX_THREAD_COUNT_SETTING, | |||
| EngineConfig.INDEX_CODEC_SETTING); | |||
| EngineConfig.INDEX_CODEC_SETTING, | |||
| DataTier.TIER_PREFERENCE_SETTING); | |||
There was a problem hiding this comment.
I am not entirely sure this is right. Or rather, I think this is what we would like it to be, but I wonder if this changes behavior and whether we should consider it breaking. You mentioned that a test would fail without this, can you point me to the test and/or test-failure?
There was a problem hiding this comment.
It's https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/TransportResumeFollowActionTests.java#L207-L231 -- TransportResumeFollowActionTests#testDynamicIndexSettingsAreClassified.
My thinking (which could be wrong) is that this isn't actually a change in runtime behavior, it's just that previously the setting was invisible to the test because it was registered elsewhere. But like I said, I could be mistaken.
There was a problem hiding this comment.
If I am mistaken, though, +1 that we don't want to do a change in behavior on this PR.
There was a problem hiding this comment.
I added a test in #79031 demonstrating that we do replicate this setting today (which is what I suspected). However, these settings are in fact only copied over on the initial bootstrap follow. They are however not updated while following the index. So adding tier preference here is fine.
In fact, it is already covered by IndexMetadata.INDEX_ROUTING_INCLUDE_GROUP_SETTING, which also matches _tier_preference due to the common prefix.
Add a test demonstrating that the follower receives the tier preference from the leader index. Relates elastic#78995
| @@ -444,7 +445,8 @@ private static ShardFollowTask createShardFollowTask( | |||
| MergeSchedulerConfig.AUTO_THROTTLE_SETTING, | |||
| MergeSchedulerConfig.MAX_MERGE_COUNT_SETTING, | |||
| MergeSchedulerConfig.MAX_THREAD_COUNT_SETTING, | |||
| EngineConfig.INDEX_CODEC_SETTING); | |||
| EngineConfig.INDEX_CODEC_SETTING, | |||
| DataTier.TIER_PREFERENCE_SETTING); | |||
There was a problem hiding this comment.
I added a test in #79031 demonstrating that we do replicate this setting today (which is what I suspected). However, these settings are in fact only copied over on the initial bootstrap follow. They are however not updated while following the index. So adding tier preference here is fine.
In fact, it is already covered by IndexMetadata.INDEX_ROUTING_INCLUDE_GROUP_SETTING, which also matches _tier_preference due to the common prefix.
Add a test demonstrating that the follower receives the tier preference from the leader index. Relates #78995
Add a test demonstrating that the follower receives the tier preference from the leader index. Relates #78995
Follow up to #78668, but also related to #76147
It threw me off that we're still registering this via
XPackPlugin, so here's a PR that stops doing that. I wish I'd caught this for #78880, but I didn't.I'm not entirely sure this is a good idea, perhaps there were complicated reasons why it had to stay where it was.