From 90a03a029bd995d56889e0917d133134ede7269a Mon Sep 17 00:00:00 2001 From: Shivansh Arora Date: Wed, 20 Sep 2023 11:46:46 +0530 Subject: [PATCH] Modified NodeShardStates to only have getter method Signed-off-by: Shivansh Arora --- .../gateway/PrimaryShardAllocator.java | 109 ++++++------------ 1 file changed, 36 insertions(+), 73 deletions(-) diff --git a/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java b/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java index b60fd8d63bf3b..06c208f48717a 100644 --- a/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/PrimaryShardAllocator.java @@ -143,7 +143,7 @@ public AllocateUnassignedDecision makeAllocationDecision( private static NodeShardStates adaptToNodeShardStates(FetchResult shardsState) { NodeShardStates nodeShardStates = new NodeShardStates(); shardsState.getData().forEach((node, nodeGatewayStartedShard) -> { - nodeShardStates.add( + nodeShardStates.getNodeShardStates().add( new NodeShardState( node, nodeGatewayStartedShard.allocationId(), @@ -179,12 +179,12 @@ protected AllocateUnassignedDecision getAllocationDecision( shardState, logger ); - final boolean enoughAllocationsFound = nodeShardsResult.orderedAllocationCandidates.size() > 0; + final boolean enoughAllocationsFound = !nodeShardsResult.orderedAllocationCandidates.getNodeShardStates().isEmpty(); logger.debug( "[{}][{}]: found {} allocation candidates of {} based on allocation ids: [{}]", unassignedShard.index(), unassignedShard.id(), - nodeShardsResult.orderedAllocationCandidates.size(), + nodeShardsResult.orderedAllocationCandidates.getNodeShardStates().size(), unassignedShard, inSyncAllocationIds ); @@ -323,15 +323,15 @@ private static List buildNodeDecisions( }) .collect(Collectors.toList()) ); - fetchedShardData.iterator().forEachRemaining(shardData -> { - if (discoNodes.contains(shardData.getNode()) == false) { - ineligibleShards.add(shardData); - } - }); + ineligibleShards = fetchedShardData + .getNodeShardStates() + .stream() + .filter(shardData -> discoNodes.contains(shardData.getNode()) == false) + .collect(Collectors.toList()); } else { // there were no shard copies that were eligible for being assigned the allocation, // so all fetched shard data are ineligible shards - fetchedShardData.iterator().forEachRemaining(ineligibleShards::add); + ineligibleShards = fetchedShardData.getNodeShardStates(); } nodeResults.addAll( @@ -374,9 +374,7 @@ protected NodeShardsResult buildNodeShardsResult( ) { NodeShardStates nodeShardStates = new NodeShardStates(); int numberOfAllocationsFound = 0; - Iterator iterator = shardState.iterator(); - while (iterator.hasNext()) { - NodeShardState nodeShardState = iterator.next(); + for (NodeShardState nodeShardState : shardState.getNodeShardStates()) { DiscoveryNode node = nodeShardState.getNode(); String allocationId = nodeShardState.allocationId(); @@ -394,24 +392,24 @@ protected NodeShardsResult buildNodeShardsResult( final String finalAllocationId = allocationId; if (nodeShardState.storeException() instanceof ShardLockObtainFailedException) { logger.trace( - () -> new ParameterizedMessage( - "[{}] on node [{}] has allocation id [{}] but the store can not be " - + "opened as it's locked, treating as valid shard", - shard, - nodeShardState.getNode(), - finalAllocationId - ), - nodeShardState.storeException() + () -> new ParameterizedMessage( + "[{}] on node [{}] has allocation id [{}] but the store can not be " + + "opened as it's locked, treating as valid shard", + shard, + nodeShardState.getNode(), + finalAllocationId + ), + nodeShardState.storeException() ); } else { logger.trace( - () -> new ParameterizedMessage( - "[{}] on node [{}] has allocation id [{}] but the store can not be " + "opened, treating as no allocation id", - shard, - nodeShardState.getNode(), - finalAllocationId - ), - nodeShardState.storeException() + () -> new ParameterizedMessage( + "[{}] on node [{}] has allocation id [{}] but the store can not be " + "opened, treating as no allocation id", + shard, + nodeShardState.getNode(), + finalAllocationId + ), + nodeShardState.storeException() ); allocationId = null; } @@ -419,23 +417,23 @@ protected NodeShardsResult buildNodeShardsResult( if (allocationId != null) { assert nodeShardState.storeException() == null || nodeShardState.storeException() instanceof ShardLockObtainFailedException - : "only allow store that can be opened or that throws a ShardLockObtainFailedException while being opened but got a " + : "only allow store that can be opened or that throws a ShardLockObtainFailedException while being opened but got a " + "store throwing " + nodeShardState.storeException(); numberOfAllocationsFound++; if (matchAnyShard || inSyncAllocationIds.contains(nodeShardState.allocationId())) { - nodeShardStates.add(nodeShardState); + nodeShardStates.getNodeShardStates().add(nodeShardState); } } } - nodeShardStates.sort(createActiveShardComparator(matchAnyShard, inSyncAllocationIds)); + nodeShardStates.getNodeShardStates().sort(createActiveShardComparator(matchAnyShard, inSyncAllocationIds)); if (logger.isTraceEnabled()) { logger.trace( "{} candidates for allocation: {}", shard, - nodeShardStates.stream().map(s -> s.getNode().getName()).collect(Collectors.joining(", ")) + nodeShardStates.getNodeShardStates().stream().map(s -> s.getNode().getName()).collect(Collectors.joining(", ")) ); } return new NodeShardsResult(nodeShardStates, numberOfAllocationsFound); @@ -477,17 +475,15 @@ protected static NodesToAllocate buildNodesToAllocate( List yesNodeShards = new ArrayList<>(); List throttledNodeShards = new ArrayList<>(); List noNodeShards = new ArrayList<>(); - Iterator iterator = nodeShardStates.iterator(); - while (iterator.hasNext()) { - NodeShardState nodeShardState = iterator.next(); + for (NodeShardState nodeShardState : nodeShardStates.getNodeShardStates()) { RoutingNode node = allocation.routingNodes().node(nodeShardState.getNode().getId()); if (node == null) { continue; } Decision decision = forceAllocate - ? allocation.deciders().canForceAllocatePrimary(shardRouting, node, allocation) - : allocation.deciders().canAllocate(shardRouting, node, allocation); + ? allocation.deciders().canForceAllocatePrimary(shardRouting, node, allocation) + : allocation.deciders().canAllocate(shardRouting, node, allocation); DecidedNode decidedNode = new DecidedNode(nodeShardState, decision); if (decision.type() == Type.THROTTLE) { throttledNodeShards.add(decidedNode); @@ -647,7 +643,7 @@ public DiscoveryNode getNode() { * @see TreeMap */ protected static class NodeShardStates { - // TreeMap to store NodeShardState and DiscoveryNode pairs + // List of entries to store NodeShardState private final List nodeShardStates; /** @@ -658,43 +654,10 @@ public NodeShardStates() { } /** - * Adds a new {@link NodeShardState} to the list. - * @param state {@link NodeShardState} node shard state. - */ - public void add(NodeShardState state) { - this.nodeShardStates.add(state); - } - - /** - * Returns the number of key-value pairs in the TreeMap. - * @return Number of key-value pairs. - */ - public int size() { - return this.nodeShardStates.size(); - } - - /** - * Returns an iterator over the {@link NodeShardState} keys in the TreeMap. - * @return Iterator over the keys. - */ - public Iterator iterator() { - return this.nodeShardStates.iterator(); - } - - /** - ** Returns a stream of the {@link NodeShardState} keys in the TreeMap. - * @return Stream of the keys. - */ - public Stream stream() { - return this.nodeShardStates.stream(); - } - - /** - * Sorts the NodeShardStates based on the provided Comparator. - * @param comparator The Comparator to use. + * Returns the list of {@link NodeShardState}. */ - public void sort(Comparator comparator) { - this.nodeShardStates.sort(comparator); + public List getNodeShardStates() { + return this.nodeShardStates; } } }