Skip to content

Commit

Permalink
Refactor out deprecated AddVotingConfigExclusionsRequest constructor
Browse files Browse the repository at this point in the history
  • Loading branch information
@zacharymorn
zacharymorn committed Mar 26, 2020
1 parent aaa0f89 commit 02a3533
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 38 deletions.
9 changes: 0 additions & 9 deletions ...rg/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,6 @@ public class AddVotingConfigExclusionsRequest extends MasterNodeRequest<AddVotin
private final String[] nodeNames;
private final TimeValue timeout;

/**
* Construct a request to add voting config exclusions for master-eligible nodes matching the given descriptions, and wait for a
* default 30 seconds for these exclusions to take effect, removing the nodes from the voting configuration.
* @param nodeDescriptions Descriptions of the nodes to add - see {@link DiscoveryNodes#resolveNodes(String...)}
*/
public AddVotingConfigExclusionsRequest(String[] nodeDescriptions) {
this(nodeDescriptions, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30));
}

/**
* Construct a request to add voting config exclusions for master-eligible nodes matching the given descriptions, and wait for these
* nodes to be removed from the voting configuration.
Expand Down
17 changes: 9 additions & 8 deletions ...asticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequestTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,15 @@ public void testResolve() {
final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).nodes(new Builder()
.add(localNode).add(otherNode1).add(otherNode2).add(otherDataNode).localNodeId(localNode.getId())).build();

assertThat(makeRequest("_all").resolveVotingConfigExclusions(clusterState),
assertThat(makeRequestWithNodeDescriptions("_all").resolveVotingConfigExclusions(clusterState),
containsInAnyOrder(localNodeExclusion, otherNode1Exclusion, otherNode2Exclusion));
assertThat(makeRequest("_local").resolveVotingConfigExclusions(clusterState),
assertThat(makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusions(clusterState),
contains(localNodeExclusion));
assertThat(makeRequest("other*").resolveVotingConfigExclusions(clusterState),
assertThat(makeRequestWithNodeDescriptions("other*").resolveVotingConfigExclusions(clusterState),
containsInAnyOrder(otherNode1Exclusion, otherNode2Exclusion));

assertThat(expectThrows(IllegalArgumentException.class,
() -> makeRequest("not-a-node").resolveVotingConfigExclusions(clusterState)).getMessage(),
() -> makeRequestWithNodeDescriptions("not-a-node").resolveVotingConfigExclusions(clusterState)).getMessage(),
equalTo("add voting config exclusions request for [not-a-node] matched no master-eligible nodes"));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}
Expand Down Expand Up @@ -316,16 +316,17 @@ public void testResolveAndCheckMaximum() {
.coordinationMetaData(CoordinationMetaData.builder().addVotingConfigExclusion(otherNode1Exclusion).build()));
final ClusterState clusterState = builder.build();

assertThat(makeRequest("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 2, "setting.name"),
assertThat(makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 2, "setting.name"),
contains(localNodeExclusion));
assertThat(expectThrows(IllegalArgumentException.class,
() -> makeRequest("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 1, "setting.name")).getMessage(),
() -> makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 1, "setting.name"))
.getMessage(),
equalTo("add voting config exclusions request for [_local] would add [1] exclusions to the existing [1] which would " +
"exceed the maximum of [1] set by [setting.name]"));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}

private static AddVotingConfigExclusionsRequest makeRequest(String... descriptions) {
return new AddVotingConfigExclusionsRequest(descriptions);
private static AddVotingConfigExclusionsRequest makeRequestWithNodeDescriptions(String... descriptions) {
return new AddVotingConfigExclusionsRequest(descriptions, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30));
}
}
35 changes: 20 additions & 15 deletions ...rch/action/admin/cluster/configuration/TransportAddVotingConfigExclusionsActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ public void testWithdrawsVoteFromANode() throws InterruptedException {

clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions());
transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"other1"}),
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY,
new String[]{"other1"}, TimeValue.timeValueSeconds(30)),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -149,15 +150,15 @@ public void testWithdrawsVoteFromANode() throws InterruptedException {

assertTrue(countDownLatch.await(30, TimeUnit.SECONDS));
assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), contains(otherNode1Exclusion));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}

public void testWithdrawsVotesFromMultipleNodes() throws InterruptedException {
final CountDownLatch countDownLatch = new CountDownLatch(1);

clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions());
transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"other1", "other2"}),
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY,
new String[]{"other1", "other2"}, TimeValue.timeValueSeconds(30)),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -167,15 +168,15 @@ public void testWithdrawsVotesFromMultipleNodes() throws InterruptedException {
assertTrue(countDownLatch.await(30, TimeUnit.SECONDS));
assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(),
containsInAnyOrder(otherNode1Exclusion, otherNode2Exclusion));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}

public void testWithdrawsVotesFromNodesMatchingWildcard() throws InterruptedException {
final CountDownLatch countDownLatch = new CountDownLatch(1);

clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions());
transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"other*"}),
new AddVotingConfigExclusionsRequest(new String[]{"other*"}, Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -193,7 +194,8 @@ public void testWithdrawsVotesFromAllMasterEligibleNodes() throws InterruptedExc

clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions());
transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"_all"}),
new AddVotingConfigExclusionsRequest(new String[]{"_all"}, Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -211,7 +213,8 @@ public void testWithdrawsVoteFromLocalNode() throws InterruptedException {

clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions());
transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"_local"}),
new AddVotingConfigExclusionsRequest(new String[]{"_local"}, Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -236,7 +239,7 @@ public void testReturnsImmediatelyIfVoteAlreadyWithdrawn() throws InterruptedExc

// no observer to reconfigure
transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"other1"}, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, TimeValue.ZERO),
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, new String[]{"other1"}, TimeValue.ZERO),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -246,15 +249,15 @@ public void testReturnsImmediatelyIfVoteAlreadyWithdrawn() throws InterruptedExc
assertTrue(countDownLatch.await(30, TimeUnit.SECONDS));
assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(),
contains(otherNode1Exclusion));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}

public void testReturnsErrorIfNoMatchingNodes() throws InterruptedException {
public void testReturnsErrorIfNoMatchingNodesWithDeprecatedNodeDescriptions() throws InterruptedException {
final CountDownLatch countDownLatch = new CountDownLatch(1);
final SetOnce<TransportException> exceptionHolder = new SetOnce<>();

transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"not-a-node"}),
new AddVotingConfigExclusionsRequest(new String[]{"not-a-node"}, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY,
TimeValue.timeValueSeconds(30)),
expectError(e -> {
exceptionHolder.set(e);
countDownLatch.countDown();
Expand All @@ -274,7 +277,8 @@ public void testOnlyMatchesMasterEligibleNodes() throws InterruptedException {
final SetOnce<TransportException> exceptionHolder = new SetOnce<>();

transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"_all", "master:false"}),
new AddVotingConfigExclusionsRequest(new String[]{"_all", "master:false"}, Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)),
expectError(e -> {
exceptionHolder.set(e);
countDownLatch.countDown();
Expand Down Expand Up @@ -372,7 +376,8 @@ public void testSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedExce
final CountDownLatch countDownLatch = new CountDownLatch(1);

transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"other1"}),
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY,
new String[]{"other1"}, TimeValue.timeValueSeconds(30)),
expectSuccess(r -> {
assertNotNull(r);
countDownLatch.countDown();
Expand All @@ -382,7 +387,6 @@ public void testSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedExce
assertTrue(countDownLatch.await(30, TimeUnit.SECONDS));
assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(),
contains(otherNode1Exclusion));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}

public void testExcludeByNodeIdSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedException {
Expand Down Expand Up @@ -464,7 +468,8 @@ public void testReturnsErrorIfMaximumExclusionCountExceeded() throws Interrupted
final SetOnce<TransportException> exceptionHolder = new SetOnce<>();

transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME,
new AddVotingConfigExclusionsRequest(new String[]{"other*"}),
new AddVotingConfigExclusionsRequest(new String[]{"other*"}, Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)),
expectError(e -> {
exceptionHolder.set(e);
countDownLatch.countDown();
Expand Down
8 changes: 6 additions & 2 deletions server/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.plugins.Plugin;
Expand Down Expand Up @@ -123,7 +125,8 @@ public void testTwoNodesNoMasterBlock() throws Exception {
String masterNode = internalCluster().getMasterName();
String otherNode = node1Name.equals(masterNode) ? node2Name : node1Name;
logger.info("--> add voting config exclusion for non-master node, to be sure it's not elected");
client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(new String[]{otherNode})).get();
client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, new String[]{otherNode}, TimeValue.timeValueSeconds(30))).get();
logger.info("--> stop master node, no master block should appear");
Settings masterDataPathSettings = internalCluster().dataPathSettings(masterNode);
internalCluster().stopRandomNode(InternalTestCluster.nameFilter(masterNode));
Expand Down Expand Up @@ -170,7 +173,8 @@ public void testTwoNodesNoMasterBlock() throws Exception {
masterNode = internalCluster().getMasterName();
otherNode = node1Name.equals(masterNode) ? node2Name : node1Name;
logger.info("--> add voting config exclusion for master node, to be sure it's not elected");
client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(new String[]{masterNode})).get();
client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, new String[]{masterNode}, TimeValue.timeValueSeconds(30))).get();
logger.info("--> stop non-master node, no master block should appear");
Settings otherNodeDataPathSettings = internalCluster().dataPathSettings(otherNode);
internalCluster().stopRandomNode(InternalTestCluster.nameFilter(otherNode));
Expand Down
5 changes: 4 additions & 1 deletion server/src/test/java/org/elasticsearch/cluster/SpecificMasterNodesIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
import org.apache.lucene.search.join.ScoreMode;
import org.elasticsearch.action.admin.cluster.configuration.AddVotingConfigExclusionsAction;
import org.elasticsearch.action.admin.cluster.configuration.AddVotingConfigExclusionsRequest;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.discovery.MasterNotDiscoveredException;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.node.Node;
Expand Down Expand Up @@ -115,7 +117,8 @@ public void testElectOnlyBetweenMasterNodes() throws Exception {

logger.info("--> closing master node (1)");
client().execute(AddVotingConfigExclusionsAction.INSTANCE,
new AddVotingConfigExclusionsRequest(new String[]{masterNodeName})).get();
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, new String[]{masterNodeName},
TimeValue.timeValueSeconds(30))).get();
// removing the master from the voting configuration immediately triggers the master to step down
assertBusy(() -> {
assertThat(internalCluster().nonMasterClient().admin().cluster().prepareState()
Expand Down
5 changes: 4 additions & 1 deletion server/src/test/java/org/elasticsearch/cluster/coordination/VotingConfigurationIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.transport.MockTransportService;
Expand Down Expand Up @@ -55,7 +57,8 @@ public void testAbdicateAfterVotingConfigExclusionAdded() throws ExecutionExcept

logger.info("--> excluding master node {}", originalMaster);
client().execute(AddVotingConfigExclusionsAction.INSTANCE,
new AddVotingConfigExclusionsRequest(new String[]{originalMaster})).get();
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY,
new String[]{originalMaster}, TimeValue.timeValueSeconds(30))).get();
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).get();
assertNotEquals(originalMaster, internalCluster().getMasterName());
}
Expand Down
4 changes: 3 additions & 1 deletion server/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.index.Index;
Expand Down Expand Up @@ -309,7 +310,8 @@ public void testTwoNodeFirstNodeCleared() throws Exception {

Map<String, long[]> primaryTerms = assertAndCapturePrimaryTerms(null);

client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(new String[]{firstNode})).get();
client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY, new String[]{firstNode}, TimeValue.timeValueSeconds(30))).get();

internalCluster().fullRestart(new RestartCallback() {
@Override
Expand Down
3 changes: 2 additions & 1 deletion test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1673,7 +1673,8 @@ private Set<String> excludeMasters(Collection<NodeAndClient> nodeAndClients) {
logger.info("adding voting config exclusions {} prior to restart/shutdown", excludedNodeIds);
try {
client().execute(AddVotingConfigExclusionsAction.INSTANCE,
new AddVotingConfigExclusionsRequest(excludedNodeIds.toArray(Strings.EMPTY_ARRAY))).get();
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, excludedNodeIds.toArray(Strings.EMPTY_ARRAY),
Strings.EMPTY_ARRAY, timeValueSeconds(30))).get();
} catch (InterruptedException | ExecutionException e) {
throw new AssertionError("unexpected", e);
}
Expand Down

0 comments on commit 02a3533

Please sign in to comment.