Revert "[Bugfix] Fix NPE in ReplicaShardAllocator (opensearch-project…

…#13993) (opensearch-project#14385)" This reverts commit cf0d6cc Signed-off-by: Shivansh Arora <[email protected]>
shiv0408 · Oct 17, 2024 · 87664f9 · 87664f9
1 parent e7ad37d
commit 87664f9
Show file tree

Hide file tree

Showing 2 changed files with 1 addition and 58 deletions.
diff --git a/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java b/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java
@@ -100,10 +100,7 @@ protected Runnable cancelExistingRecoveryForBetterMatch(
         Metadata metadata = allocation.metadata();
         RoutingNodes routingNodes = allocation.routingNodes();
         ShardRouting primaryShard = allocation.routingNodes().activePrimary(shard.shardId());
-        if (primaryShard == null) {
-            logger.trace("{}: no active primary shard found or allocated, letting actual allocation figure it out", shard);
-            return null;
-        }
+        assert primaryShard != null : "the replica shard can be allocated on at least one node, so there must be an active primary";
         assert primaryShard.currentNodeId() != null;
         final DiscoveryNode primaryNode = allocation.nodes().get(primaryShard.currentNodeId());
 

diff --git a/server/src/test/java/org/opensearch/gateway/ReplicaShardBatchAllocatorTests.java b/server/src/test/java/org/opensearch/gateway/ReplicaShardBatchAllocatorTests.java
@@ -644,25 +644,6 @@ public void testDoNotCancelForBrokenNode() {
         assertThat(allocation.routingNodes().shardsWithState(ShardRoutingState.UNASSIGNED), empty());
     }
 
-    public void testDoNotCancelForInactivePrimaryNode() {
-        RoutingAllocation allocation = oneInactivePrimaryOnNode1And1ReplicaRecovering(yesAllocationDeciders(), null);
-        testBatchAllocator.addData(
-            node1,
-            null,
-            "MATCH",
-            null,
-            new StoreFileMetadata("file1", 10, "MATCH_CHECKSUM", MIN_SUPPORTED_LUCENE_VERSION)
-        ).addData(node2, randomSyncId(), null, new StoreFileMetadata("file1", 10, "MATCH_CHECKSUM", MIN_SUPPORTED_LUCENE_VERSION));
-
-        testBatchAllocator.processExistingRecoveries(
-            allocation,
-            Collections.singletonList(new ArrayList<>(allocation.routingNodes().shardsWithState(ShardRoutingState.INITIALIZING)))
-        );
-
-        assertThat(allocation.routingNodesChanged(), equalTo(false));
-        assertThat(allocation.routingNodes().shardsWithState(ShardRoutingState.UNASSIGNED), empty());
-    }
-
     public void testAllocateUnassignedBatchThrottlingAllocationDeciderIsHonoured() throws InterruptedException {
         ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
         AllocationDeciders allocationDeciders = randomAllocationDeciders(
@@ -936,41 +917,6 @@ private RoutingAllocation onePrimaryOnNode1And1ReplicaRecovering(AllocationDecid
         );
     }
 
-    private RoutingAllocation oneInactivePrimaryOnNode1And1ReplicaRecovering(AllocationDeciders deciders, UnassignedInfo unassignedInfo) {
-        ShardRouting primaryShard = TestShardRouting.newShardRouting(shardId, node1.getId(), true, ShardRoutingState.INITIALIZING);
-        RoutingTable routingTable = RoutingTable.builder()
-            .add(
-                IndexRoutingTable.builder(shardId.getIndex())
-                    .addIndexShard(
-                        new IndexShardRoutingTable.Builder(shardId).addShard(primaryShard)
-                            .addShard(
-                                TestShardRouting.newShardRouting(
-                                    shardId,
-                                    node2.getId(),
-                                    null,
-                                    false,
-                                    ShardRoutingState.INITIALIZING,
-                                    unassignedInfo
-                                )
-                            )
-                            .build()
-                    )
-            )
-            .build();
-        ClusterState state = ClusterState.builder(org.opensearch.cluster.ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY))
-            .routingTable(routingTable)
-            .nodes(DiscoveryNodes.builder().add(node1).add(node2))
-            .build();
-        return new RoutingAllocation(
-            deciders,
-            new RoutingNodes(state, false),
-            state,
-            ClusterInfo.EMPTY,
-            SnapshotShardSizeInfo.EMPTY,
-            System.nanoTime()
-        );
-    }
-
     private RoutingAllocation onePrimaryOnNode1And1ReplicaRecovering(AllocationDeciders deciders) {
         return onePrimaryOnNode1And1ReplicaRecovering(deciders, new UnassignedInfo(UnassignedInfo.Reason.CLUSTER_RECOVERED, null));
     }