From 474060ef0586e2d4fb85de84dade3d4d3df72e2a Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 13 Aug 2024 22:57:44 +0530 Subject: [PATCH] Refactor Signed-off-by: Rishab Nahata --- .../gateway/BaseGatewayShardAllocator.java | 18 ++---------------- .../gateway/PrimaryShardAllocator.java | 12 +++++++++++- .../gateway/ReplicaShardAllocator.java | 12 +++++++++++- .../gateway/ReplicaShardBatchAllocator.java | 2 +- 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/opensearch/gateway/BaseGatewayShardAllocator.java b/server/src/main/java/org/opensearch/gateway/BaseGatewayShardAllocator.java index 38561f63e3d92..2b6c5e3f5ae53 100644 --- a/server/src/main/java/org/opensearch/gateway/BaseGatewayShardAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/BaseGatewayShardAllocator.java @@ -38,7 +38,6 @@ import org.opensearch.cluster.routing.RoutingNode; import org.opensearch.cluster.routing.RoutingNodes; import org.opensearch.cluster.routing.ShardRouting; -import org.opensearch.cluster.routing.UnassignedInfo; import org.opensearch.cluster.routing.allocation.AllocateUnassignedDecision; import org.opensearch.cluster.routing.allocation.AllocationDecision; import org.opensearch.cluster.routing.allocation.ExistingShardsAllocator; @@ -91,7 +90,7 @@ protected void allocateUnassignedBatchOnTimeout(Set shardIds, RoutingAl ShardRouting unassignedShard = iterator.next(); AllocateUnassignedDecision allocationDecision; if (unassignedShard.primary() == primary && shardIds.contains(unassignedShard.shardId())) { - if (isResponsibleFor(unassignedShard, primary) == false) { + if (isResponsibleFor(unassignedShard) == false) { continue; } allocationDecision = AllocateUnassignedDecision.throttle(null); @@ -103,20 +102,7 @@ protected void allocateUnassignedBatchOnTimeout(Set shardIds, RoutingAl /** * Is the allocator responsible for allocating the given {@link ShardRouting}? */ - protected static boolean isResponsibleFor(final ShardRouting shard, boolean primary) { - if (primary) { - return shard.primary() // must be primary - && shard.unassigned() // must be unassigned - // only handle either an existing store or a snapshot recovery - && (shard.recoverySource().getType() == RecoverySource.Type.EXISTING_STORE - || shard.recoverySource().getType() == RecoverySource.Type.SNAPSHOT); - } else { - return shard.primary() == false // must be a replica - && shard.unassigned() // must be unassigned - // if we are allocating a replica because of index creation, no need to go and find a copy, there isn't one... - && shard.unassignedInfo().getReason() != UnassignedInfo.Reason.INDEX_CREATED; - } - } + protected abstract boolean isResponsibleFor(ShardRouting shardRouting); protected void executeDecision( ShardRouting shardRouting, diff --git a/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java b/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java index bb504df3fa44c..dea7ca9a08edd 100644 --- a/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java @@ -79,6 +79,16 @@ * @opensearch.internal */ public abstract class PrimaryShardAllocator extends BaseGatewayShardAllocator { + /** + * Is the allocator responsible for allocating the given {@link ShardRouting}? + */ + protected boolean isResponsibleFor(final ShardRouting shard) { + return shard.primary() // must be primary + && shard.unassigned() // must be unassigned + // only handle either an existing store or a snapshot recovery + && (shard.recoverySource().getType() == RecoverySource.Type.EXISTING_STORE + || shard.recoverySource().getType() == RecoverySource.Type.SNAPSHOT); + } /** * Skip doing fetchData call for a shard if recovery mode is snapshot. Also do not take decision if allocator is @@ -89,7 +99,7 @@ public abstract class PrimaryShardAllocator extends BaseGatewayShardAllocator { * @return allocation decision taken for this shard */ protected AllocateUnassignedDecision getInEligibleShardDecision(ShardRouting unassignedShard, RoutingAllocation allocation) { - if (isResponsibleFor(unassignedShard, true) == false) { + if (isResponsibleFor(unassignedShard) == false) { // this allocator is not responsible for allocating this shard return AllocateUnassignedDecision.NOT_TAKEN; } diff --git a/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java b/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java index 5f08c898f38a2..c30ee8479ac97 100644 --- a/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java @@ -188,13 +188,23 @@ public void processExistingRecoveries(RoutingAllocation allocation) { } } + /** + * Is the allocator responsible for allocating the given {@link ShardRouting}? + */ + protected boolean isResponsibleFor(final ShardRouting shard) { + return shard.primary() == false // must be a replica + && shard.unassigned() // must be unassigned + // if we are allocating a replica because of index creation, no need to go and find a copy, there isn't one... + && shard.unassignedInfo().getReason() != UnassignedInfo.Reason.INDEX_CREATED; + } + @Override public AllocateUnassignedDecision makeAllocationDecision( final ShardRouting unassignedShard, final RoutingAllocation allocation, final Logger logger ) { - if (isResponsibleFor(unassignedShard, false) == false) { + if (isResponsibleFor(unassignedShard) == false) { // this allocator is not responsible for deciding on this shard return AllocateUnassignedDecision.NOT_TAKEN; } diff --git a/server/src/main/java/org/opensearch/gateway/ReplicaShardBatchAllocator.java b/server/src/main/java/org/opensearch/gateway/ReplicaShardBatchAllocator.java index d9e1512f1bba8..020a543ac5fc5 100644 --- a/server/src/main/java/org/opensearch/gateway/ReplicaShardBatchAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/ReplicaShardBatchAllocator.java @@ -173,7 +173,7 @@ private AllocateUnassignedDecision getUnassignedShardAllocationDecision( RoutingAllocation allocation, Supplier> nodeStoreFileMetaDataMapSupplier ) { - if (!isResponsibleFor(shardRouting, false)) { + if (isResponsibleFor(shardRouting) == false) { return AllocateUnassignedDecision.NOT_TAKEN; } Tuple> result = canBeAllocatedToAtLeastOneNode(shardRouting, allocation);