Add a new node role 'cluster_manager' as the alternative for 'master' role and deprecate 'master' role #2424
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,13 +34,16 @@ | |
|
|
||
| import org.opensearch.LegacyESVersion; | ||
| import org.opensearch.Version; | ||
| import org.opensearch.common.logging.DeprecationLogger; | ||
| import org.opensearch.common.settings.Setting; | ||
| import org.opensearch.common.settings.Setting.Property; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.transport.RemoteClusterService; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Objects; | ||
| import java.util.SortedSet; | ||
| import java.util.TreeSet; | ||
|
|
@@ -50,6 +53,10 @@ | |
| */ | ||
| public abstract class DiscoveryNodeRole implements Comparable<DiscoveryNodeRole> { | ||
|
|
||
| private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(DiscoveryNodeRole.class); | ||
| public static final String MASTER_ROLE_DEPRECATION_MESSAGE = | ||
| "Assigning [master] role in setting [node.roles] is deprecated. To promote inclusive language, please use [cluster_manager] role instead."; | ||
|
|
||
| private final String roleName; | ||
|
|
||
| /** | ||
|
|
@@ -129,6 +136,13 @@ public DiscoveryNodeRole getCompatibilityRole(Version nodeVersion) { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Validate the role is compatible with the other roles in the list, when assigning the list of roles to a node. | ||
| * An {@link IllegalArgumentException} is expected to be thrown, if the role can't coexist with the other roles. | ||
| * @param roles A {@link List} of {@link DiscoveryNodeRole} that a node is going to have. | ||
| */ | ||
| public void validateRole(List<DiscoveryNodeRole> roles) {}; | ||
|
|
||
| @Override | ||
| public final boolean equals(Object o) { | ||
| if (this == o) return true; | ||
|
|
@@ -193,15 +207,60 @@ public Setting<Boolean> legacySetting() { | |
|
|
||
| /** | ||
| * Represents the role for a master-eligible node. | ||
| * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #CLUSTER_MANAGER_ROLE} | ||
| */ | ||
| @Deprecated | ||
| public static final DiscoveryNodeRole MASTER_ROLE = new DiscoveryNodeRole("master", "m") { | ||
|
|
||
| @Override | ||
| public Setting<Boolean> legacySetting() { | ||
| // copy the setting here so we can mark it private in org.opensearch.node.Node | ||
| // As of 2.0, set the default value to 'false', so that MASTER_ROLE isn't added as a default value of NODE_ROLES_SETTING | ||
| return Setting.boolSetting("node.master", false, Property.Deprecated, Property.NodeScope); | ||
| } | ||
|
|
||
| @Override | ||
| public void validateRole(List<DiscoveryNodeRole> roles) { | ||
| deprecationLogger.deprecate("node_role_master", MASTER_ROLE_DEPRECATION_MESSAGE); | ||
| } | ||
|
|
||
| }; | ||
|
|
||
| /** | ||
| * Represents the role for a cluster-manager-eligible node. | ||
| */ | ||
| public static final DiscoveryNodeRole CLUSTER_MANAGER_ROLE = new DiscoveryNodeRole("cluster_manager", "m") { | ||
|
|
||
| @Override | ||
| public Setting<Boolean> legacySetting() { | ||
| // copy the setting here so we can mark it private in org.opensearch.node.Node | ||
| return Setting.boolSetting("node.master", true, Property.Deprecated, Property.NodeScope); | ||
| } | ||
|
|
||
| @Override | ||
| public DiscoveryNodeRole getCompatibilityRole(Version nodeVersion) { | ||
| if (nodeVersion.onOrAfter(Version.V_2_0_0)) { | ||
| return this; | ||
| } else { | ||
| return DiscoveryNodeRole.MASTER_ROLE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void validateRole(List<DiscoveryNodeRole> roles) { | ||
| if (roles.contains(DiscoveryNodeRole.MASTER_ROLE)) { | ||
| throw new IllegalArgumentException( | ||
| String.format( | ||
| Locale.ROOT, | ||
| "The two roles [%s, %s] can not be assigned together to a node. %s", | ||
| DiscoveryNodeRole.MASTER_ROLE.roleName(), | ||
| DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), | ||
| MASTER_ROLE_DEPRECATION_MESSAGE | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. @andrross Thanks for your opinion to not allow "master" role and "cluster_manager" role coexisting. 👍 😄 It's achieved by adding a method validateRole() to allow defining different validation rule for each role, to check the compatibility with the other roles in the setting |
||
|
|
||
| }; | ||
|
|
||
| public static final DiscoveryNodeRole REMOTE_CLUSTER_CLIENT_ROLE = new DiscoveryNodeRole("remote_cluster_client", "r") { | ||
|
|
@@ -223,7 +282,7 @@ public Setting<Boolean> legacySetting() { | |
| * The built-in node roles. | ||
| */ | ||
| public static SortedSet<DiscoveryNodeRole> BUILT_IN_ROLES = Collections.unmodifiableSortedSet( | ||
| new TreeSet<>(Arrays.asList(DATA_ROLE, INGEST_ROLE, CLUSTER_MANAGER_ROLE, REMOTE_CLUSTER_CLIENT_ROLE)) | ||
| ); | ||
|
|
||
| /** | ||
|
|
@@ -262,4 +321,13 @@ public Setting<Boolean> legacySetting() { | |
|
|
||
| } | ||
|
|
||
| /** | ||
| * Check if the role is {@link #CLUSTER_MANAGER_ROLE} or {@link #MASTER_ROLE}. | ||
| * @deprecated As of 2.0, because promoting inclusive language. MASTER_ROLE is deprecated. | ||
| * @return true if the node role is{@link #CLUSTER_MANAGER_ROLE} or {@link #MASTER_ROLE} | ||
| */ | ||
| @Deprecated | ||
| public boolean isClusterManager() { | ||
| return this.equals(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE) || this.equals(DiscoveryNodeRole.MASTER_ROLE); | ||
| } | ||
| } | ||
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
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.
isMasterNode -> isClusterManager?
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.
Such method is published into Maven as Java API (https://opensearch.org/javadocs/1.2.4/OpenSearch/server/build/docs/javadoc/org/opensearch/cluster/node/DiscoveryNode.html#isMasterNode()), so I'm not going to change the name soon, in case any plugins or clients are using the method, though maybe no other software using it 😅.
I think the normal way is to deprecate this method and create new method aside, but it will be a huge work to deprecate all the methods/classes with the mane "master", so I'm planning to rename them in place in next major version.
Of course, I think there is an option to choose a few common used methods to follow the normal deprecation path. I haven't got a decision which method to rename in place, and which to create alternative method aside.
There is a discussion in the PR #2453, you could take a look at. Renaming the Java APIs is tracked in issue #1684
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.
Deprecating isMaster in favour of isClusterManager sounds like a plan
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 will mark it in an issue and deprecate it another PR. Thank you!