-
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
[TEST] Add back necessary tests for deprecated settings that are replaced during applying inclusive naming #2825
Changes from 12 commits
334c59d
c57b695
90cc894
69b5757
b15a558
1a6e3f3
f9eefb7
e5d730a
9bc46f4
eb54608
c64878f
76a20eb
5fe6e1a
5722253
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -450,6 +450,65 @@ public void testResolveAndCheckMaximum() { | |
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); | ||
} | ||
|
||
// As of 2.0, MASTER_ROLE is deprecated to promote inclusive language. | ||
// Validate node with MASTER_ROLE can be resolved by resolveVotingConfigExclusions() like before. | ||
// TODO: Remove the test after removing MASTER_ROLE. | ||
public void testResolveByNodeDescriptionWithDeprecatedMasterRole() { | ||
final DiscoveryNode localNode = new DiscoveryNode( | ||
"local", | ||
"local", | ||
buildNewFakeTransportAddress(), | ||
emptyMap(), | ||
singleton(DiscoveryNodeRole.MASTER_ROLE), | ||
Version.CURRENT | ||
); | ||
final VotingConfigExclusion localNodeExclusion = new VotingConfigExclusion(localNode); | ||
|
||
final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")) | ||
.nodes(new Builder().add(localNode).localNodeId(localNode.getId())) | ||
.build(); | ||
|
||
assertThat(makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusions(clusterState), contains(localNodeExclusion)); | ||
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion seems unnecessary for this test case - you can probably remove it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👏 You are so careful that notice such detail! Good catch, I will try to replace with |
||
} | ||
|
||
// TODO: Remove the test after removing MASTER_ROLE. | ||
public void testResolveByNodeIdWithDeprecatedMasterRole() { | ||
final DiscoveryNode node = new DiscoveryNode( | ||
"nodeName", | ||
"nodeId", | ||
buildNewFakeTransportAddress(), | ||
emptyMap(), | ||
singleton(DiscoveryNodeRole.MASTER_ROLE), | ||
Version.CURRENT | ||
); | ||
final VotingConfigExclusion nodeExclusion = new VotingConfigExclusion(node); | ||
|
||
final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).nodes(new Builder().add(node)).build(); | ||
|
||
assertThat( | ||
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[] { "nodeId" }, Strings.EMPTY_ARRAY, TimeValue.ZERO) | ||
.resolveVotingConfigExclusions(clusterState), | ||
contains(nodeExclusion) | ||
); | ||
} | ||
|
||
public void testResolveByNodeNameWithDeprecatedMasterRole() { | ||
final DiscoveryNode node = new DiscoveryNode( | ||
"nodeName", | ||
"nodeId", | ||
buildNewFakeTransportAddress(), | ||
emptyMap(), | ||
singleton(DiscoveryNodeRole.MASTER_ROLE), | ||
Version.CURRENT | ||
); | ||
final VotingConfigExclusion nodeExclusion = new VotingConfigExclusion(node); | ||
|
||
final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).nodes(new Builder().add(node)).build(); | ||
|
||
assertThat(new AddVotingConfigExclusionsRequest("nodeName").resolveVotingConfigExclusions(clusterState), contains(nodeExclusion)); | ||
} | ||
|
||
private static AddVotingConfigExclusionsRequest makeRequestWithNodeDescriptions(String... nodeDescriptions) { | ||
return new AddVotingConfigExclusionsRequest( | ||
nodeDescriptions, | ||
|
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 - I don't think we need these TODOs alongside all of the tests. When we remove
DiscoveryNodeRole.MASTER_ROLE
these tests would automatically break, so we would anyway resolve them thenThere 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 for your review! 👍 I will remove the unnecessary TODOs.
The reason I added them is to giving a clear mind that it definitely can be removed. 😂
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.
Removed all the TODO in this PR, and I believe the comment of every newly added test is enough to make people understand they are only used to test the deprecated "master" role setting.