Add entry to Deprecation API for CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING#73552
Conversation
…ELOCATIONS_SETTING Relates elastic#47717
fcofdez
added
:Distributed/Allocation
|
Pinging @elastic/es-distributed (Team:Distributed) |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
The setting is dynamic and I wonder if we need a cluster check too. While the setting will be archived, behavior will change if the setting is set to false in cluster settings.
Let me follow-up on that outside this issue.
|
|
||
| public void testClusterRoutingAllocationIncludeRelocationsSetting() { | ||
| Settings settings = Settings.builder() | ||
| .put(CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.getKey(), false).build(); |
There was a problem hiding this comment.
I think both setting to false and true is deprecated, so suggest to randomize:
| .put(CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.getKey(), false).build(); | |
| .put(CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.getKey(), randomBoolean()).build(); |
|
|
||
| static DeprecationIssue checkClusterRoutingAllocationIncludeRelocationsSetting(final Settings settings, | ||
| final PluginsAndModules pluginsAndModules) { | ||
| if (CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.exists(settings)) { |
There was a problem hiding this comment.
Can we reuse checkRemovedSetting here?
…nclude-relocations-config
| return checkRemovedSetting(settings, | ||
| CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING, | ||
| "https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-8.0.html#breaking_80_allocation_changes", | ||
| DeprecationIssue.Level.WARNING |
There was a problem hiding this comment.
I think it's CRITICAL at least in the case where it's in the node settings. If it's set dynamically then that's technically ok to proceed, we'll just archive it, but that's also a case we can fix at runtime so maybe not so bad to call it CRITICAL too? Will the upgrade assistant automatically help users remove this setting if set dynamically or do we need to take action on that front too?
There was a problem hiding this comment.
I've changed it to CRITICAL when it's in the node settings. I think we could tackle the case when the setting is set dynamically with the upgrade assistant, we just need to open a ticket in Kibana describing how the assistant can mitigate the problem. I'll open the ticket.
|
@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
LGTM, left an optional comment (can be tackled in a follow-up too if you prefer).
| // The setting is dynamic so it can be defined in the ClusterState settings | ||
| return getClusterRoutingAllocationIncludeRelocationsSettingDeprecationIssue( | ||
| clusterState.metadata().settings(), | ||
| DeprecationIssue.Level.WARNING | ||
| ); |
There was a problem hiding this comment.
I would prefer to have this part of the check added separately to DeprecationChecks.CLUSTER_SETTINGS_CHECKS, to avoid having a deprecation issue per node.
…nclude-relocations-config
…nclude-relocations-config
|
@elasticmachine run elasticsearch-ci/part-2 |
4 participants
Relates #47717