Browse Source

`IndexShardRoutingTable` should always be nonempty (#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.
David Turner 1 year ago
parent
commit
c8a35d349c

+ 3 - 3
server/src/main/java/org/elasticsearch/action/get/TransportGetAction.java

@@ -296,11 +296,11 @@ public class TransportGetAction extends TransportSingleShardAction<GetRequest, G
     }
 
     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;
     }

+ 1 - 1
server/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java

@@ -848,7 +848,7 @@ public abstract class TransportReplicationAction<
                     : "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 [{}]",

+ 2 - 1
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);

+ 3 - 4
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

@@ -1189,7 +1189,7 @@ public final class SnapshotsService extends AbstractLifecycleComponent implement
                 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 @@ public final class SnapshotsService extends AbstractLifecycleComponent implement
                                 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;
                             }
                         }

+ 7 - 4
server/src/test/java/org/elasticsearch/cluster/routing/IndexShardRoutingTableTests.java

@@ -12,7 +12,6 @@ import org.elasticsearch.index.Index;
 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 class IndexShardRoutingTableTests extends ESTestCase {
         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);