Rename InternalTestCluster#getMasterNodeInstance (#83407)

DaveCTurner · DaveCTurner · commit c22f77e7a598 · 2022-02-08T15:07:56.000+01:00
This method's name is trappy: it is easy to misinterpret it as returning
an instance from the elected master, but in fact it uses any
master-eligible node. If you want an instance from the elected master,
you have to use `getCurrentMasterNodeInstance()` instead.

This commit renames the method to clarify that it might not get an
instance from the elected master, and adds docs with cross-refs to help
developers choose the right method.
diff --git a/modules/repository-gcs/src/internalClusterTest/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/modules/repository-gcs/src/internalClusterTest/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java
@@ -119,7 +119,7 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
 
     public void testDeleteSingleItem() {
         final String repoName = createRepository(randomRepositoryName());
-        final RepositoriesService repositoriesService = internalCluster().getMasterNodeInstance(RepositoriesService.class);
+        final RepositoriesService repositoriesService = internalCluster().getAnyMasterNodeInstance(RepositoriesService.class);
         final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(repoName);
         PlainActionFuture.get(
             f -> repository.threadPool()
diff --git a/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestGetMappingsCancellationIT.java b/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestGetMappingsCancellationIT.java
@@ -96,7 +96,7 @@ public TimeValue masterNodeTimeout() {
         };
 
         PlainActionFuture<AcknowledgedResponse> future = PlainActionFuture.newFuture();
-        internalCluster().getMasterNodeInstance(ClusterService.class)
+        internalCluster().getAnyMasterNodeInstance(ClusterService.class)
             .submitStateUpdateTask("get_mappings_cancellation_test", new AckedClusterStateUpdateTask(ackedRequest, future) {
                 @Override
                 public ClusterState execute(ClusterState currentState) throws Exception {
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java b/server/src/internalClusterTest/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java
@@ -153,7 +153,7 @@ public void testBootstrapNoClusterState() throws IOException {
         String node = internalCluster().startNode();
         Settings dataPathSettings = internalCluster().dataPathSettings(node);
         ensureStableCluster(1);
-        NodeEnvironment nodeEnvironment = internalCluster().getMasterNodeInstance(NodeEnvironment.class);
+        NodeEnvironment nodeEnvironment = internalCluster().getAnyMasterNodeInstance(NodeEnvironment.class);
         internalCluster().stopRandomDataNode();
         Environment environment = TestEnvironment.newEnvironment(
             Settings.builder().put(internalCluster().getDefaultSettings()).put(dataPathSettings).build()
@@ -168,7 +168,7 @@ public void testDetachNoClusterState() throws IOException {
         String node = internalCluster().startNode();
         Settings dataPathSettings = internalCluster().dataPathSettings(node);
         ensureStableCluster(1);
-        NodeEnvironment nodeEnvironment = internalCluster().getMasterNodeInstance(NodeEnvironment.class);
+        NodeEnvironment nodeEnvironment = internalCluster().getAnyMasterNodeInstance(NodeEnvironment.class);
         internalCluster().stopRandomDataNode();
         Environment environment = TestEnvironment.newEnvironment(
             Settings.builder().put(internalCluster().getDefaultSettings()).put(dataPathSettings).build()
@@ -257,7 +257,7 @@ public void test3MasterNodes2Failed() throws Exception {
         expectThrows(() -> unsafeBootstrap(environmentMaster1), UnsafeBootstrapMasterCommand.FAILED_TO_OBTAIN_NODE_LOCK_MSG);
 
         logger.info("--> stop 1st master-eligible node and data-only node");
-        NodeEnvironment nodeEnvironment = internalCluster().getMasterNodeInstance(NodeEnvironment.class);
+        NodeEnvironment nodeEnvironment = internalCluster().getAnyMasterNodeInstance(NodeEnvironment.class);
         internalCluster().stopRandomNode(InternalTestCluster.nameFilter(masterNodes.get(0)));
         assertBusy(() -> internalCluster().getInstance(GatewayMetaState.class, dataNode).allPendingAsyncStatesWritten());
         internalCluster().stopRandomDataNode();
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java
@@ -698,7 +698,7 @@ public void testSnapshotRecovery() throws Exception {
         logger.info("--> request recoveries");
         RecoveryResponse response = client().admin().indices().prepareRecoveries(INDEX_NAME).execute().actionGet();
 
-        Repository repository = internalCluster().getMasterNodeInstance(RepositoriesService.class).repository(REPO_NAME);
+        Repository repository = internalCluster().getAnyMasterNodeInstance(RepositoriesService.class).repository(REPO_NAME);
         final RepositoryData repositoryData = PlainActionFuture.get(repository::getRepositoryData);
         for (Map.Entry<String, List<RecoveryState>> indexRecoveryStates : response.shardRecoveryStates().entrySet()) {
 
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsServiceIT.java
@@ -281,9 +281,9 @@ public void testFetchingInformationFromAnIncompatibleMasterNodeReturnsAnEmptyLis
         createRepository(repositoryName, "fs", randomRepoPath(), true);
         createSnapshot(repositoryName, snapshotName, indexName);
 
-        RepositoriesService repositoriesService = internalCluster().getMasterNodeInstance(RepositoriesService.class);
-        ThreadPool threadPool = internalCluster().getMasterNodeInstance(ThreadPool.class);
-        ClusterService clusterService = internalCluster().getMasterNodeInstance(ClusterService.class);
+        RepositoriesService repositoriesService = internalCluster().getAnyMasterNodeInstance(RepositoriesService.class);
+        ThreadPool threadPool = internalCluster().getAnyMasterNodeInstance(ThreadPool.class);
+        ClusterService clusterService = internalCluster().getAnyMasterNodeInstance(ClusterService.class);
         ShardSnapshotsService shardSnapshotsService = new ShardSnapshotsService(client(), repositoriesService, threadPool, clusterService) {
             @Override
             protected boolean masterSupportsFetchingLatestSnapshots() {
@@ -305,9 +305,9 @@ private Optional<ShardSnapshot> getLatestShardSnapshot(ShardId shardId) throws E
     }
 
     private ShardSnapshotsService getShardSnapshotsService() {
-        RepositoriesService repositoriesService = internalCluster().getMasterNodeInstance(RepositoriesService.class);
-        ThreadPool threadPool = internalCluster().getMasterNodeInstance(ThreadPool.class);
-        ClusterService clusterService = internalCluster().getMasterNodeInstance(ClusterService.class);
+        RepositoriesService repositoriesService = internalCluster().getAnyMasterNodeInstance(RepositoriesService.class);
+        ThreadPool threadPool = internalCluster().getAnyMasterNodeInstance(ThreadPool.class);
+        ClusterService clusterService = internalCluster().getAnyMasterNodeInstance(ClusterService.class);
         return new ShardSnapshotsService(client(), repositoriesService, threadPool, clusterService);
     }
 
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java
@@ -812,7 +812,7 @@ private void runTestDefaultNumberOfReplicasTest(final boolean closeIndex) {
 
     public void testNoopUpdate() {
         internalCluster().ensureAtLeastNumDataNodes(2);
-        final ClusterService clusterService = internalCluster().getMasterNodeInstance(ClusterService.class);
+        final ClusterService clusterService = internalCluster().getAnyMasterNodeInstance(ClusterService.class);
         assertAcked(
             client().admin()
                 .indices()
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/persistent/PersistentTaskInitializationFailureIT.java b/server/src/internalClusterTest/java/org/elasticsearch/persistent/PersistentTaskInitializationFailureIT.java
@@ -53,7 +53,7 @@ public void testPersistentTasksThatFailDuringInitializationAreRemovedFromCluster
         startPersistentTaskFuture.actionGet();
 
         assertBusy(() -> {
-            final ClusterService clusterService = internalCluster().getMasterNodeInstance(ClusterService.class);
+            final ClusterService clusterService = internalCluster().getAnyMasterNodeInstance(ClusterService.class);
             final PersistentTasksCustomMetadata persistentTasks = clusterService.state()
                 .metadata()
                 .custom(PersistentTasksCustomMetadata.TYPE);
diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESBlobStoreRepositoryIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESBlobStoreRepositoryIntegTestCase.java
@@ -269,7 +269,7 @@ protected BlobStore newBlobStore() {
     }
 
     protected BlobStore newBlobStore(String repository) {
-        final BlobStoreRepository blobStoreRepository = (BlobStoreRepository) internalCluster().getMasterNodeInstance(
+        final BlobStoreRepository blobStoreRepository = (BlobStoreRepository) internalCluster().getAnyMasterNodeInstance(
             RepositoriesService.class
         ).repository(repository);
         return PlainActionFuture.get(
diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java
@@ -1545,6 +1545,9 @@ public <T> Iterable<T> getDataNodeInstances(Class<T> clazz) {
         return getInstances(clazz, DATA_NODE_PREDICATE);
     }
 
+    /**
+     * Return the instance of the given class &gt;T&lt; from the elected master node. See also {@link #getAnyMasterNodeInstance}.
+     */
     public synchronized <T> T getCurrentMasterNodeInstance(Class<T> clazz) {
         return getInstance(clazz, new NodeNamePredicate(getMasterName()));
     }
@@ -1577,7 +1580,11 @@ public <T> T getDataNodeInstance(Class<T> clazz) {
         return getInstance(clazz, DATA_NODE_PREDICATE);
     }
 
-    public <T> T getMasterNodeInstance(Class<T> clazz) {
+    /**
+     * Return the instance of the given class &gt;T&lt; from any of the master-eligible nodes (not necessarily the elected master). See
+     * also {@link #getCurrentMasterNodeInstance}.
+     */
+    public <T> T getAnyMasterNodeInstance(Class<T> clazz) {
         return getInstance(clazz, MASTER_NODE_PREDICATE);
     }
 
diff --git a/x-pack/plugin/autoscaling/src/internalClusterTest/java/org/elasticsearch/xpack/autoscaling/action/GetAutoscalingCapacityRestCancellationIT.java b/x-pack/plugin/autoscaling/src/internalClusterTest/java/org/elasticsearch/xpack/autoscaling/action/GetAutoscalingCapacityRestCancellationIT.java
@@ -83,27 +83,27 @@ public void testCapacityRestCancellationAndResponse() throws Exception {
             PlainActionFuture<Response> successFuture2 = new PlainActionFuture<>();
             Request getCapacityRequest = new Request("GET", "/_autoscaling/capacity");
             Cancellable cancellable = restClient.performRequestAsync(getCapacityRequest, wrapAsRestResponseListener(cancelledFuture));
-            LocalStateAutoscaling.AutoscalingTestPlugin plugin = internalCluster().getMasterNodeInstance(PluginsService.class)
+            LocalStateAutoscaling.AutoscalingTestPlugin plugin = internalCluster().getAnyMasterNodeInstance(PluginsService.class)
                 .filterPlugins(LocalStateAutoscaling.class)
                 .get(0)
                 .testPlugin();
             plugin.syncWithDeciderService(() -> {
                 putAutoscalingPolicy(Map.of(AutoscalingCountTestDeciderService.NAME, Settings.EMPTY));
                 assertThat(
-                    internalCluster().getMasterNodeInstance(TransportGetAutoscalingCapacityAction.class).responseCacheQueueSize(),
+                    internalCluster().getAnyMasterNodeInstance(TransportGetAutoscalingCapacityAction.class).responseCacheQueueSize(),
                     equalTo(1)
                 );
                 restClient.performRequestAsync(getCapacityRequest, wrapAsRestResponseListener(successFuture1));
                 assertBusy(
                     () -> assertThat(
-                        internalCluster().getMasterNodeInstance(TransportGetAutoscalingCapacityAction.class).responseCacheQueueSize(),
+                        internalCluster().getAnyMasterNodeInstance(TransportGetAutoscalingCapacityAction.class).responseCacheQueueSize(),
                         equalTo(2)
                     )
                 );
                 restClient.performRequestAsync(getCapacityRequest, wrapAsRestResponseListener(successFuture2));
                 assertBusy(
                     () -> assertThat(
-                        internalCluster().getMasterNodeInstance(TransportGetAutoscalingCapacityAction.class).responseCacheQueueSize(),
+                        internalCluster().getAnyMasterNodeInstance(TransportGetAutoscalingCapacityAction.class).responseCacheQueueSize(),
                         equalTo(3)
                     )
                 );
@@ -130,7 +130,7 @@ public void testCapacityRestCancellationAndResponse() throws Exception {
 
     private void waitForCancelledCapacityTask() throws Exception {
         assertBusy(() -> {
-            TransportService transportService = internalCluster().getMasterNodeInstance(TransportService.class);
+            TransportService transportService = internalCluster().getAnyMasterNodeInstance(TransportService.class);
             final TaskManager taskManager = transportService.getTaskManager();
             assertTrue(taskManager.assertCancellableTaskConsistency());
             for (CancellableTask cancellableTask : taskManager.getCancellableTasks().values()) {
diff --git a/x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/RestartIndexFollowingIT.java b/x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/RestartIndexFollowingIT.java
@@ -117,7 +117,7 @@ public void testFollowIndex() throws Exception {
 
     private void setupRemoteCluster() throws Exception {
         ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest().masterNodeTimeout(TimeValue.MAX_VALUE);
-        String address = getLeaderCluster().getMasterNodeInstance(TransportService.class).boundAddress().publishAddress().toString();
+        String address = getLeaderCluster().getAnyMasterNodeInstance(TransportService.class).boundAddress().publishAddress().toString();
         updateSettingsRequest.persistentSettings(Settings.builder().put("cluster.remote.leader_cluster.seeds", address));
         assertAcked(followerClient().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
         List<RemoteConnectionInfo> infos = followerClient().execute(RemoteInfoAction.INSTANCE, new RemoteInfoRequest()).get().getInfos();

Original file line number	Diff line number	Diff line change
`@@ -269,7 +269,7 @@ protected BlobStore newBlobStore() {`
`269`	`269`	`}`
`270`	`270`
`271`	`271`	`protected BlobStore newBlobStore(String repository) {`
`272`		`- final BlobStoreRepository blobStoreRepository = (BlobStoreRepository) internalCluster().getMasterNodeInstance(`
	`272`	`+ final BlobStoreRepository blobStoreRepository = (BlobStoreRepository) internalCluster().getAnyMasterNodeInstance(`
`273`	`273`	`RepositoriesService.class`
`274`	`274`	`).repository(repository);`
`275`	`275`	`return PlainActionFuture.get(`
Original file line number	Diff line number	Diff line change
`@@ -1545,6 +1545,9 @@ public <T> Iterable<T> getDataNodeInstances(Class<T> clazz) {`
`1545`	`1545`	`return getInstances(clazz, DATA_NODE_PREDICATE);`
`1546`	`1546`	`}`
`1547`	`1547`
	`1548`	`+ /**`
	`1549`	`+ * Return the instance of the given class >T< from the elected master node. See also {@link #getAnyMasterNodeInstance}.`
	`1550`	`+ */`
`1548`	`1551`	`public synchronized <T> T getCurrentMasterNodeInstance(Class<T> clazz) {`
`1549`	`1552`	`return getInstance(clazz, new NodeNamePredicate(getMasterName()));`
`1550`	`1553`	`}`
`@@ -1577,7 +1580,11 @@ public <T> T getDataNodeInstance(Class<T> clazz) {`
`1577`	`1580`	`return getInstance(clazz, DATA_NODE_PREDICATE);`
`1578`	`1581`	`}`
`1579`	`1582`
`1580`		`- public <T> T getMasterNodeInstance(Class<T> clazz) {`
	`1583`	`+ /**`
	`1584`	`+ * Return the instance of the given class >T< from any of the master-eligible nodes (not necessarily the elected master). See`
	`1585`	`+ * also {@link #getCurrentMasterNodeInstance}.`
	`1586`	`+ */`
	`1587`	`+ public <T> T getAnyMasterNodeInstance(Class<T> clazz) {`
`1581`	`1588`	`return getInstance(clazz, MASTER_NODE_PREDICATE);`
`1582`	`1589`	`}`
`1583`	`1590`