Some cleanups to OperationRouting (#119440)

Two small cleanups that remove duplication and dead-code:
1. The shardId method can be made `static`, it's only non-static use in mocking was unnecessary and could just be deleted
2. Filtering for searchable shards is the same logic now for stateful and stateless. We also only use it when building a
filtered `PlainShardIterator` so it might as well live there to remove some duplication.

Author: original-brownbear

server/src/internalClusterTest/java/org/elasticsearch/recovery/RecoveryWhileUnderLoadIT.java

@@ -16,8 +16,8 @@
 import org.elasticsearch.action.support.broadcast.BroadcastResponse;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
+import org.elasticsearch.cluster.routing.OperationRouting;
 import org.elasticsearch.cluster.routing.ShardRouting;
-import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.util.CollectionUtils;
@@ -365,11 +365,10 @@ private void iterateAssertCount(final int numberOfShards, final int iterations,
                 );
             }
 
-            ClusterService clusterService = clusterService();
-            final ClusterState state = clusterService.state();
+            final ClusterState state = clusterService().state();
             for (int shard = 0; shard < numberOfShards; shard++) {
                 for (String id : ids) {
-                    ShardId docShard = clusterService.operationRouting().shardId(state, "test", id, null);
+                    ShardId docShard = OperationRouting.shardId(state, "test", id, null);
                     if (docShard.id() == shard) {
                         final IndexShardRoutingTable indexShardRoutingTable = state.routingTable().shardRoutingTable("test", shard);
                         for (int copy = 0; copy < indexShardRoutingTable.size(); copy++) {

server/src/main/java/org/elasticsearch/action/fieldcaps/RequestDispatcher.java

@@ -95,7 +95,7 @@ final class RequestDispatcher {
         for (String index : indices) {
             final GroupShardsIterator<ShardIterator> shardIts;
             try {
-                shardIts = clusterService.operationRouting().searchShards(clusterState, new String[] { index }, null, null, null, null);
+                shardIts = clusterService.operationRouting().searchShards(clusterState, new String[] { index }, null, null);
             } catch (Exception e) {
                 onIndexFailure.accept(index, e);
                 continue;

server/src/main/java/org/elasticsearch/action/get/TransportGetAction.java

@@ -30,7 +30,6 @@
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.routing.PlainShardIterator;
 import org.elasticsearch.cluster.routing.ShardIterator;
-import org.elasticsearch.cluster.routing.ShardRouting;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.core.TimeValue;
@@ -109,7 +108,7 @@ protected ShardIterator shards(ClusterState state, InternalRequest request) {
         if (iterator == null) {
             return null;
         }
-        return new PlainShardIterator(iterator.shardId(), iterator.getShardRoutings().stream().filter(ShardRouting::isSearchable).toList());
+        return PlainShardIterator.allSearchableShards(iterator);
     }
 
     @Override

server/src/main/java/org/elasticsearch/action/get/TransportMultiGetAction.java

@@ -19,6 +19,7 @@
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.block.ClusterBlockLevel;
 import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
+import org.elasticsearch.cluster.routing.OperationRouting;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.util.concurrent.AtomicArray;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
@@ -81,7 +82,7 @@ protected void doExecute(Task task, final MultiGetRequest request, final ActionL
                     lastResolvedIndex = Tuple.tuple(item.index(), concreteSingleIndex);
                 }
                 item.routing(clusterState.metadata().resolveIndexRouting(item.routing(), item.index()));
-                shardId = clusterService.operationRouting().shardId(clusterState, concreteSingleIndex, item.id(), item.routing());
+                shardId = OperationRouting.shardId(clusterState, concreteSingleIndex, item.id(), item.routing());
             } catch (RoutingMissingException e) {
                 responses.set(i, newItemFailure(e.getIndex().getName(), e.getId(), e));
                 continue;

server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java

@@ -30,7 +30,6 @@
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.routing.PlainShardIterator;
 import org.elasticsearch.cluster.routing.ShardIterator;
-import org.elasticsearch.cluster.routing.ShardRouting;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.core.TimeValue;
@@ -113,7 +112,7 @@ protected ShardIterator shards(ClusterState state, InternalRequest request) {
         if (iterator == null) {
             return null;
         }
-        return new PlainShardIterator(iterator.shardId(), iterator.getShardRoutings().stream().filter(ShardRouting::isSearchable).toList());
+        return PlainShardIterator.allSearchableShards(iterator);
     }
 
     @Override

server/src/main/java/org/elasticsearch/action/termvectors/TransportMultiTermVectorsAction.java

@@ -17,6 +17,7 @@
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.block.ClusterBlockLevel;
 import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
+import org.elasticsearch.cluster.routing.OperationRouting;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.util.concurrent.AtomicArray;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
@@ -72,8 +73,12 @@ protected void doExecute(Task task, final MultiTermVectorsRequest request, final
                     clusterState.metadata().resolveIndexRouting(termVectorsRequest.routing(), termVectorsRequest.index())
                 );
                 String concreteSingleIndex = indexNameExpressionResolver.concreteSingleIndex(clusterState, termVectorsRequest).getName();
-                shardId = clusterService.operationRouting()
-                    .shardId(clusterState, concreteSingleIndex, termVectorsRequest.id(), termVectorsRequest.routing());
+                shardId = OperationRouting.shardId(
+                    clusterState,
+                    concreteSingleIndex,
+                    termVectorsRequest.id(),
+                    termVectorsRequest.routing()
+                );
             } catch (RoutingMissingException e) {
                 responses.set(
                     i,

server/src/main/java/org/elasticsearch/action/termvectors/TransportTermVectorsAction.java

@@ -62,16 +62,27 @@ public TransportTermVectorsAction(
 
     @Override
     protected ShardIterator shards(ClusterState state, InternalRequest request) {
+        final var operationRouting = clusterService.operationRouting();
         if (request.request().doc() != null && request.request().routing() == null) {
             // artificial document without routing specified, ignore its "id" and use either random shard or according to preference
-            GroupShardsIterator<ShardIterator> groupShardsIter = clusterService.operationRouting()
-                .searchShards(state, new String[] { request.concreteIndex() }, null, request.request().preference());
+            GroupShardsIterator<ShardIterator> groupShardsIter = operationRouting.searchShards(
+                state,
+                new String[] { request.concreteIndex() },
+                null,
+                request.request().preference()
+            );
             return groupShardsIter.iterator().next();
         }
 
-        ShardIterator shards = clusterService.operationRouting()
-            .getShards(state, request.concreteIndex(), request.request().id(), request.request().routing(), request.request().preference());
-        return clusterService.operationRouting().useOnlyPromotableShardsForStateless(shards);
+        return operationRouting.useOnlyPromotableShardsForStateless(
+            operationRouting.getShards(
+                state,
+                request.concreteIndex(),
+                request.request().id(),
+                request.request().routing(),
+                request.request().preference()
+            )
+        );
     }
 
     @Override

server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java

@@ -27,7 +27,6 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -125,32 +124,12 @@ public GroupShardsIterator<ShardIterator> searchShards(
                 nodeCounts
             );
             if (iterator != null) {
-                final List<ShardRouting> shardsThatCanHandleSearches;
-                if (isStateless) {
-                    shardsThatCanHandleSearches = statelessShardsThatHandleSearches(clusterState, iterator);
-                } else {
-                    shardsThatCanHandleSearches = statefulShardsThatHandleSearches(iterator);
-                }
-                set.add(new PlainShardIterator(iterator.shardId(), shardsThatCanHandleSearches));
+                set.add(PlainShardIterator.allSearchableShards(iterator));
             }
         }
         return GroupShardsIterator.sortAndCreate(new ArrayList<>(set));
     }
 
-    private static List<ShardRouting> statefulShardsThatHandleSearches(ShardIterator iterator) {
-        final List<ShardRouting> shardsThatCanHandleSearches = new ArrayList<>(iterator.size());
-        for (ShardRouting shardRouting : iterator) {
-            if (shardRouting.isSearchable()) {
-                shardsThatCanHandleSearches.add(shardRouting);
-            }
-        }
-        return shardsThatCanHandleSearches;
-    }
-
-    private static List<ShardRouting> statelessShardsThatHandleSearches(ClusterState clusterState, ShardIterator iterator) {
-        return iterator.getShardRoutings().stream().filter(ShardRouting::isSearchable).toList();
-    }
-
     public static ShardIterator getShards(ClusterState clusterState, ShardId shardId) {
         final IndexShardRoutingTable shard = clusterState.routingTable().shardRoutingTable(shardId);
         return shard.activeInitializingShardsRandomIt();
@@ -297,7 +276,7 @@ private static IndexMetadata indexMetadata(ClusterState clusterState, String ind
         return indexMetadata;
     }
 
-    public ShardId shardId(ClusterState clusterState, String index, String id, @Nullable String routing) {
+    public static ShardId shardId(ClusterState clusterState, String index, String id, @Nullable String routing) {
         IndexMetadata indexMetadata = indexMetadata(clusterState, index);
         return new ShardId(indexMetadata.getIndex(), IndexRouting.fromIndexMetadata(indexMetadata).getShard(id, routing));
     }

server/src/main/java/org/elasticsearch/cluster/routing/PlainShardIterator.java

@@ -11,6 +11,7 @@
 
 import org.elasticsearch.index.shard.ShardId;
 
+import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -21,6 +22,20 @@ public class PlainShardIterator extends PlainShardsIterator implements ShardIter
 
     private final ShardId shardId;
 
+    public static PlainShardIterator allSearchableShards(ShardIterator shardIterator) {
+        return new PlainShardIterator(shardIterator.shardId(), shardsThatCanHandleSearches(shardIterator));
+    }
+
+    private static List<ShardRouting> shardsThatCanHandleSearches(ShardIterator iterator) {
+        final List<ShardRouting> shardsThatCanHandleSearches = new ArrayList<>(iterator.size());
+        for (ShardRouting shardRouting : iterator) {
+            if (shardRouting.isSearchable()) {
+                shardsThatCanHandleSearches.add(shardRouting);
+            }
+        }
+        return shardsThatCanHandleSearches;
+    }
+
     /**
      * Creates a {@link PlainShardIterator} instance that iterates over a subset of the given shards
      * this the a given <code>shardId</code>.

server/src/test/java/org/elasticsearch/action/get/TransportMultiGetActionTests.java

@@ -145,15 +145,9 @@ public static void beforeClass() throws Exception {
         when(
             operationRouting.getShards(eq(clusterState), eq(index1.getName()), anyString(), nullable(String.class), nullable(String.class))
         ).thenReturn(index1ShardIterator);
-        when(operationRouting.shardId(eq(clusterState), eq(index1.getName()), nullable(String.class), nullable(String.class))).thenReturn(
-            new ShardId(index1, randomInt())
-        );
         when(
             operationRouting.getShards(eq(clusterState), eq(index2.getName()), anyString(), nullable(String.class), nullable(String.class))
         ).thenReturn(index2ShardIterator);
-        when(operationRouting.shardId(eq(clusterState), eq(index2.getName()), nullable(String.class), nullable(String.class))).thenReturn(
-            new ShardId(index2, randomInt())
-        );
 
         clusterService = mock(ClusterService.class);
         when(clusterService.localNode()).thenReturn(transportService.getLocalNode());