IndexShardRoutingTable should always be nonempty (elastic#105720)

There's only one remaining test that creates an empty `IndexShardRoutingTable`. This commit fixes that, and then adds an assertion to enforce that all `IndexShardRoutingTable` instances include at least a primary shard.
droberts195 · Feb 22, 2024 · c8a35d3 · c8a35d3
1 parent d842ce9
commit c8a35d3
Show file tree

Hide file tree

Showing 5 changed files with 16 additions and 13 deletions.
diff --git a/server/src/main/java/org/elasticsearch/action/get/TransportGetAction.java b/server/src/main/java/org/elasticsearch/action/get/TransportGetAction.java
@@ -296,11 +296,11 @@ private void tryGetFromTranslog(GetRequest request, IndexShard indexShard, Disco
     }
 
     static DiscoveryNode getCurrentNodeOfPrimary(ClusterState clusterState, ShardId shardId) {
-        var shardRoutingTable = clusterState.routingTable().shardRoutingTable(shardId);
-        if (shardRoutingTable.primaryShard() == null || shardRoutingTable.primaryShard().active() == false) {
+        final var primaryShard = clusterState.routingTable().shardRoutingTable(shardId).primaryShard();
+        if (primaryShard.active() == false) {
             throw new NoShardAvailableActionException(shardId, "primary shard is not active");
         }
-        DiscoveryNode node = clusterState.nodes().get(shardRoutingTable.primaryShard().currentNodeId());
+        DiscoveryNode node = clusterState.nodes().get(primaryShard.currentNodeId());
         assert node != null;
         return node;
     }

diff --git a/...rc/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java b/...rc/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java
@@ -848,7 +848,7 @@ protected void doRun() {
                     : "request waitForActiveShards must be set in resolveRequest";
 
                 final ShardRouting primary = state.getRoutingTable().shardRoutingTable(request.shardId()).primaryShard();
-                if (primary == null || primary.active() == false) {
+                if (primary.active() == false) {
                     logger.trace(
                         "primary shard [{}] is not yet active, scheduling a retry: action [{}], request [{}], "
                             + "cluster state version [{}]",

diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java b/server/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java
@@ -114,7 +114,8 @@ public class IndexShardRoutingTable {
                 allShardsStarted = false;
             }
         }
-        assert primary != null || shards.isEmpty() : shards;
+        assert shards.isEmpty() == false : "cannot have an empty shard routing table";
+        assert primary != null : shards;
         this.primary = primary;
         this.replicas = CollectionUtils.wrapUnmodifiableOrEmptySingleton(replicas);
         this.activeShards = CollectionUtils.wrapUnmodifiableOrEmptySingleton(activeShards);

diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
@@ -1189,7 +1189,7 @@ private static ImmutableOpenMap<ShardId, ShardSnapshotStatus> processWaitingShar
                 IndexRoutingTable indexShardRoutingTable = routingTable.index(shardId.getIndex());
                 if (indexShardRoutingTable != null) {
                     IndexShardRoutingTable shardRouting = indexShardRoutingTable.shard(shardId.id());
-                    if (shardRouting != null && shardRouting.primaryShard() != null) {
+                    if (shardRouting != null) {
                         final var primaryNodeId = shardRouting.primaryShard().currentNodeId();
                         if (nodeIdRemovalPredicate.test(primaryNodeId)) {
                             if (shardStatus.state() == ShardState.PAUSED_FOR_NODE_REMOVAL) {
@@ -1274,9 +1274,8 @@ private static boolean waitingShardsStartedOrUnassigned(SnapshotsInProgress snap
                                 return true;
                             }
                             ShardRouting shardRouting = indexShardRoutingTable.shard(shardId.shardId()).primaryShard();
-                            if (shardRouting != null
-                                && (shardRouting.started() && snapshotsInProgress.isNodeIdForRemoval(shardRouting.currentNodeId()) == false
-                                    || shardRouting.unassigned())) {
+                            if (shardRouting.started() && snapshotsInProgress.isNodeIdForRemoval(shardRouting.currentNodeId()) == false
+                                || shardRouting.unassigned()) {
                                 return true;
                             }
                         }

diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/IndexShardRoutingTableTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/IndexShardRoutingTableTests.java
@@ -12,7 +12,6 @@
 import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.test.ESTestCase;
 
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
@@ -35,9 +34,13 @@ public void testEquals() {
         Index index = new Index("a", "b");
         ShardId shardId = new ShardId(index, 1);
         ShardId shardId2 = new ShardId(index, 2);
-        IndexShardRoutingTable table1 = new IndexShardRoutingTable(shardId, new ArrayList<>());
-        IndexShardRoutingTable table2 = new IndexShardRoutingTable(shardId, new ArrayList<>());
-        IndexShardRoutingTable table3 = new IndexShardRoutingTable(shardId2, new ArrayList<>());
+        ShardRouting shardRouting = TestShardRouting.newShardRouting(shardId, null, true, ShardRoutingState.UNASSIGNED);
+        IndexShardRoutingTable table1 = new IndexShardRoutingTable(shardId, List.of(shardRouting));
+        IndexShardRoutingTable table2 = new IndexShardRoutingTable(shardId, List.of(shardRouting));
+        IndexShardRoutingTable table3 = new IndexShardRoutingTable(
+            shardId2,
+            List.of(TestShardRouting.newShardRouting(shardId2, null, true, ShardRoutingState.UNASSIGNED))
+        );
         String s = "Some other random object";
         assertEquals(table1, table1);
         assertEquals(table1, table2);