-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Deprecate setting 'cluster.initial_master_nodes' and introduce the alternative setting 'cluster.initial_cluster_manager_nodes' #2463
Merged
tlfeng
merged 18 commits into
opensearch-project:main
from
tlfeng:setting-initial-master-node
Mar 18, 2022
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
4ea1231
Deprecate setting cluster.initial_master_nodes, and add setting clust…
11646a4
Rename cluster.initial_master_nodes to cluster.initial_cluster_manage…
1fa949c
Replace master-eligible to cluster-manager-eligible in opensearch.yml
04ced0d
Fix bwc test by configuring setting name
cf22874
Correct a setting name in docker-compose
7d96d3f
Merge remote-tracking branch 'upstream/main' into setting-initial-mas…
cf3b6c4
Merge branch 'main' into setting-initial-master-node
e00348e
Revert changing an error message in bootstrapchecks
03e29f8
Show correct setting name in exception message
82ec6a0
Flip arguments in assertEquals(), because the first is expected value
975501f
Rename initialMasterNodes in test method name
3d1cd8e
Merge branch 'main' into setting-initial-master-node
754cd1a
Use Settings.existsOrFallbackExists() instead of exists()
1afb92b
Merge remote-tracking branch 'upstream/main' into setting-initial-mas…
c967ad4
Merge remote-tracking branch 'upstream/main' into setting-initial-mas…
2ee85da
Add both cluster.initial_cluster_manager_nodes and cluster.initial_ma…
7fc7c62
Update import not to use asterisk
bd7fdbe
Add the missing import of INITIAL_CLUSTER_MANAGER_NODES_SETTING
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
84 changes: 84 additions & 0 deletions
84
.../java/org/opensearch/cluster/coordination/ClusterBootstrapServiceRenamedSettingTests.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.cluster.coordination; | ||
|
||
import org.opensearch.common.settings.ClusterSettings; | ||
import org.opensearch.common.settings.Setting; | ||
import org.opensearch.common.settings.Settings; | ||
import org.opensearch.test.OpenSearchTestCase; | ||
|
||
import java.util.Arrays; | ||
import java.util.Set; | ||
|
||
/** | ||
* A unit test to validate the former name of the setting 'cluster.initial_cluster_manager_nodes' still take effect, | ||
* after it is deprecated, so that the backwards compatibility is maintained. | ||
* The test can be removed along with removing support of the deprecated setting. | ||
*/ | ||
public class ClusterBootstrapServiceRenamedSettingTests extends OpenSearchTestCase { | ||
/** | ||
* Validate the both settings are known and supported. | ||
*/ | ||
public void testReindexSettingsExist() { | ||
Set<Setting<?>> settings = ClusterSettings.BUILT_IN_CLUSTER_SETTINGS; | ||
assertTrue( | ||
"Both 'cluster.initial_cluster_manager_nodes' and its predecessor should be supported built-in settings.", | ||
settings.containsAll( | ||
Arrays.asList( | ||
ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING, | ||
ClusterBootstrapService.INITIAL_CLUSTER_MANAGER_NODES_SETTING | ||
) | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* Validate the default value of the both settings is the same. | ||
*/ | ||
public void testSettingFallback() { | ||
assertEquals( | ||
ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.get(Settings.EMPTY), | ||
ClusterBootstrapService.INITIAL_CLUSTER_MANAGER_NODES_SETTING.get(Settings.EMPTY) | ||
); | ||
} | ||
|
||
/** | ||
* Validate the new setting can be configured correctly, and it doesn't impact the old setting. | ||
*/ | ||
public void testSettingGetValue() { | ||
Settings settings = Settings.builder().put("cluster.initial_cluster_manager_nodes", "node-a").build(); | ||
assertEquals(Arrays.asList("node-a"), ClusterBootstrapService.INITIAL_CLUSTER_MANAGER_NODES_SETTING.get(settings)); | ||
assertEquals( | ||
ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.getDefault(Settings.EMPTY), | ||
ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.get(settings) | ||
); | ||
} | ||
|
||
/** | ||
* Validate the value of the old setting will be applied to the new setting, if the new setting is not configured. | ||
*/ | ||
public void testSettingGetValueWithFallback() { | ||
Settings settings = Settings.builder().put("cluster.initial_master_nodes", "node-a").build(); | ||
assertEquals(Arrays.asList("node-a"), ClusterBootstrapService.INITIAL_CLUSTER_MANAGER_NODES_SETTING.get(settings)); | ||
assertSettingDeprecationsAndWarnings(new Setting<?>[] { ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING }); | ||
} | ||
|
||
/** | ||
* Validate the value of the old setting will be ignored, if the new setting is configured. | ||
*/ | ||
public void testSettingGetValueWhenBothAreConfigured() { | ||
Settings settings = Settings.builder() | ||
.put("cluster.initial_cluster_manager_nodes", "node-a") | ||
.put("cluster.initial_master_nodes", "node-a, node-b") | ||
.build(); | ||
assertEquals(Arrays.asList("node-a"), ClusterBootstrapService.INITIAL_CLUSTER_MANAGER_NODES_SETTING.get(settings)); | ||
assertEquals(Arrays.asList("node-a", "node-b"), ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.get(settings)); | ||
assertSettingDeprecationsAndWarnings(new Setting<?>[] { ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING }); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nitpick - We're not branching on version here so the message may be misleading
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.
Hi @kartg , thanks a lot for your review! 👍 I got your point. I will also revise the other setting name changes in the error message.
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.
Reverted the above change in the commit e00348e.
Could I double check when the message will be misleading, if it contains the new setting name?
The situation that I could think of is: in a cluster with nodes in both 1.x and 2.x versions, if the
Bootstrapchecks
failed when a node startup, the user can only configure the setting with old name (initial_master_node
).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.
Apologies for the churn on this. You're right - keeping this message unchanged now makes it inaccurate for 2.x versions. I think the right balance is to include both settings in the error message so it reads:
You could make the argument that the master and cluster manager settings above should be separated by a
/
and not a comma, but incorporating this would make the code rather obtuse so I don't think it's worth the trouble.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.
Thank your so much for the reply @kartg ! I didn't think of this edge case of a cluster with mixed version nodes, until you reminded me.
After got your reminder, my idea was telling user use a deprecated setting name is better than telling a unsupported setting name, so I keep the old setting name.
Haha, I totally agree that the most appropriate message is
cluster.initial_cluster_manager_nodes / cluster.initial_master_nodes
, let me check the right balance, and make the change.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 added the both old and new setting name into the error message, and separate by slash
/
, in commit 2ee85da 😁