Skip to content
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

[Refactor] Not load the deprecated node role 'master' in method 'setAdditionalRoles()' which used to load roles defined by plugins #4579

Closed
tlfeng opened this issue Sep 23, 2022 · 0 comments · Fixed by #4582
Assignees
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request v2.4.0 'Issues and PRs related to version v2.4.0'

Comments

@tlfeng
Copy link
Collaborator

tlfeng commented Sep 23, 2022

Is your feature request related to a problem? Please describe.
It doesn't look like suitable to load a core node role master as "additonal roles".

In commit ea31483 / PR #2424, node role 'master' is deprecated, and the alternative role 'cluster_manager' is added.
Since the role 'cluster_manager' is the successor for 'master', the same abbreviation role name 'm' is set to the new role for convenience.
While there is a check to ensure the abbreviation names for node roles are unique:

// collect the abbreviation names into a map to ensure that there are not any duplicate abbreviations
final Map<String, DiscoveryNodeRole> roleNameAbbreviationToPossibleRoles = Collections.unmodifiableMap(
roleNameToPossibleRoles.values()
.stream()
.collect(Collectors.toMap(DiscoveryNodeRole::roleNameAbbreviation, Function.identity()))
);
assert roleNameToPossibleRoles.size() == roleNameAbbreviationToPossibleRoles.size() : "roles by name ["

As a workaround, I added the deprecated node role 'master' after the check, in the method setAdditionalRoles() which used to load roles defined by plugins, instead of BUILT_IN_ROLES:
public static SortedSet<DiscoveryNodeRole> BUILT_IN_ROLES = Collections.unmodifiableSortedSet(

public static void setAdditionalRoles(final Set<DiscoveryNodeRole> additionalRoles) {

final Set<DiscoveryNodeRole> additionalRoles = pluginsService.filterPlugins(Plugin.class)

Describe the solution you'd like
Load the deprecated master role in other places, but not in method setAdditionalRoles()

Describe alternatives you've considered
It's fine to keep as it is, because the current code of setting 'master' role is a temporary workaround and will be removed in the next major version.

Additional context

@tlfeng tlfeng added enhancement Enhancement or improvement to existing feature or request untriaged v2.4.0 'Issues and PRs related to version v2.4.0' and removed untriaged labels Sep 23, 2022
@tlfeng tlfeng self-assigned this Sep 23, 2022
@tlfeng tlfeng changed the title [Refactor] Not set the deprecated node role 'master' in method 'setAdditionalRoles()' which used to load roles defined by plugins [Refactor] Not load the deprecated node role 'master' in method 'setAdditionalRoles()' which used to load roles defined by plugins Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
2 participants