Copilot commented on code in PR #2503:
URL: https://github.com/apache/solr/pull/2503#discussion_r3682955606
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java:
##########
@@ -305,6 +312,61 @@ public BalancePlan computeBalancing(
.createBalancePlan(balanceRequest, replicaMovements);
}
+ /**
+ * Move replicas that share a node with another replica of the same shard to
other nodes, since
+ * multiple replicas of the same shard on one node give neither availability
nor capacity
+ * benefits. Placement will never create such a state, per the default {@link
+ * WeightedNode#canAddReplica(Replica)}, but users can, e.g. by adding a
replica to an explicit
+ * node. Each duplicate replica is moved to the accepting node with the
lowest projected weight
+ * with the replica added, like {@link #computePlacements(Collection,
PlacementContext)} does.
+ */
+ private static void moveDuplicateShardReplicas(
+ Collection<WeightedNode> weightedNodes, Map<Replica, Node>
replicaMovements) {
+ List<WeightedNode> sourceNodes = new ArrayList<>(weightedNodes);
+ sourceNodes.sort(Comparator.comparing(node -> node.getNode().getName()));
+ for (WeightedNode sourceNode : sourceNodes) {
+ Map<String, List<Replica>> replicasPerShard =
+ sourceNode.getAllReplicasOnNode().stream()
+ .collect(
+ Collectors.groupingBy(
+ replica ->
+ replica.getShard().getCollection().getName()
+ + "%"
+ + replica.getShard().getShardName()));
+ for (List<Replica> shardReplicas : replicasPerShard.values()) {
+ if (shardReplicas.size() < 2) {
+ continue;
+ }
+ // Keep one replica of the shard on this node and try to move the
others away, choosing
+ // replicas to move in replica name order, like the weight-based
balancing below does
+ shardReplicas.sort(Comparator.comparing(Replica::getReplicaName));
+ for (Replica replica : shardReplicas.subList(0, shardReplicas.size() -
1)) {
+ if (!sourceNode.canRemoveReplicas(Set.of(replica)).isEmpty()) {
+ continue;
+ }
+ weightedNodes.stream()
+ .filter(node -> !node.equals(sourceNode))
+ .filter(node -> node.canAddReplica(replica))
+ .min(
+ Comparator.<WeightedNode>comparingInt(
+ node -> node.calcRelevantWeightWithReplica(replica))
+ .thenComparing(Comparator.naturalOrder()))
+ .ifPresent(
+ targetNode -> {
+ log.debug(
+ "Duplicate replica movement chosen. From: {}, To: {},
Replica: {}",
+ sourceNode,
+ targetNode,
+ replica);
+ targetNode.addReplica(replica);
+ sourceNode.removeReplica(replica);
+ replicaMovements.put(replica, targetNode.getNode());
+ });
+ }
Review Comment:
The duplicate-move loop only considers `shardReplicas.subList(0,
shardReplicas.size() - 1)`. If one of those replicas cannot be removed (e.g.,
`canRemoveReplicas` rejects it), the method will never attempt moving the last
replica instead, so it can leave duplicates on the node even when a removable
replica exists. Iterate all replicas in name order and keep moving removable
ones until only one replica of the shard remains on the source node (or no
eligible target exists).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
