This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_9x in repository https://gitbox.apache.org/repos/asf/solr.git
commit 051b3883f952a496c9b5a6dc258328aa85988d6a Author: Jan Høydahl <[email protected]> AuthorDate: Mon Aug 3 09:50:18 2026 +0200 SOLR-18327: Balance replicas API does not balance duplicate replicas on same node (#2503) (cherry picked from commit 5e58e05f0f34fe2d1c878fd78a3c556297359179) --- .../balance-duplicate-replicas-SOLR-18327.yml | 9 +++ .../src/java/org/apache/solr/cluster/Shard.java | 9 +++ .../plugins/OrderedNodePlacementPlugin.java | 71 ++++++++++++++++++- .../plugins/AffinityPlacementFactoryTest.java | 81 +++++++++++++++++++++- .../plugins/MinimizeCoresPlacementFactoryTest.java | 8 ++- 5 files changed, 171 insertions(+), 7 deletions(-) diff --git a/changelog/unreleased/balance-duplicate-replicas-SOLR-18327.yml b/changelog/unreleased/balance-duplicate-replicas-SOLR-18327.yml new file mode 100644 index 00000000000..260b8c51249 --- /dev/null +++ b/changelog/unreleased/balance-duplicate-replicas-SOLR-18327.yml @@ -0,0 +1,9 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: Balance replicas API now enforces the rule of at most one replica per shard on the same node +type: fixed +authors: + - name: Jan Høydahl +links: + - name: SOLR-18327 + url: https://issues.apache.org/jira/browse/SOLR-18327 diff --git a/solr/core/src/java/org/apache/solr/cluster/Shard.java b/solr/core/src/java/org/apache/solr/cluster/Shard.java index f62443e897b..cb89e68ad6d 100644 --- a/solr/core/src/java/org/apache/solr/cluster/Shard.java +++ b/solr/core/src/java/org/apache/solr/cluster/Shard.java @@ -28,6 +28,15 @@ public interface Shard { */ SolrCollection getCollection(); + /** + * @return an id for this shard that is unique across the cluster, combining the collection name + * and the shard name. Useful e.g. as a map key when grouping replicas of the same shard, + * possibly from different collections. + */ + default String getUniqueShardId() { + return getCollection().getName() + "%" + getShardName(); + } + /** * Returns the {@link Replica} of the given name for that shard, if such a replica exists. * diff --git a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java index 86ca79526b3..b78bdb3fe1c 100644 --- a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java +++ b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java @@ -202,14 +202,21 @@ public abstract class OrderedNodePlacementPlugin implements PlacementPlugin { public BalancePlan computeBalancing( BalanceRequest balanceRequest, PlacementContext placementContext) throws PlacementException { Map<Replica, Node> replicaMovements = new HashMap<>(); - TreeSet<WeightedNode> orderedNodes = new TreeSet<>(); - orderedNodes.addAll( + Collection<WeightedNode> weightedNodes = getWeightedNodes( placementContext, balanceRequest.getNodes(), placementContext.getCluster().collections(), true) - .values()); + .values(); + + // First move replicas that share a node with another replica of the same shard, a state the + // weight-based balancing below cannot necessarily detect since such duplicates do not have to + // make their node weigh more than its peers. + moveDuplicateShardReplicas(weightedNodes, replicaMovements); + + TreeSet<WeightedNode> orderedNodes = new TreeSet<>(); + orderedNodes.addAll(weightedNodes); // While the node with the lowest weight still has room to take a replica from the node with the // highest weight, loop @@ -306,6 +313,64 @@ public abstract class OrderedNodePlacementPlugin implements PlacementPlugin { .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().getUniqueShardId())); + for (List<Replica> shardReplicas : replicasPerShard.values()) { + if (shardReplicas.size() < 2) { + continue; + } + // Move extra replicas of this shard away until only one remains on the node, in replica + // name order like the weight-based balancing below. A replica that cannot be removed or has + // no eligible target is left in place and does not stop us from moving the others, so a + // single stuck replica no longer leaves the duplicate unresolved. + shardReplicas.sort(Comparator.comparing(Replica::getReplicaName)); + int remainingOnNode = shardReplicas.size(); + for (Replica replica : shardReplicas) { + if (remainingOnNode < 2) { + break; + } + if (!sourceNode.canRemoveReplicas(Set.of(replica)).isEmpty()) { + continue; + } + Optional<WeightedNode> targetNode = + weightedNodes.stream() + .filter(node -> !node.equals(sourceNode)) + .filter(node -> node.canAddReplica(replica)) + .min( + Comparator.<WeightedNode>comparingInt( + node -> node.calcRelevantWeightWithReplica(replica)) + .thenComparing(Comparator.naturalOrder())); + if (targetNode.isPresent()) { + WeightedNode target = targetNode.get(); + log.debug( + "Duplicate replica movement chosen. From: {}, To: {}, Replica: {}", + sourceNode, + target, + replica); + target.addReplica(replica); + sourceNode.removeReplica(replica); + replicaMovements.put(replica, target.getNode()); + remainingOnNode--; + } + } + } + } + } + protected Map<Node, WeightedNode> getWeightedNodes( PlacementContext placementContext, Set<Node> nodes, diff --git a/solr/core/src/test/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactoryTest.java b/solr/core/src/test/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactoryTest.java index 424161dafa4..bfa5e91634f 100644 --- a/solr/core/src/test/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactoryTest.java +++ b/solr/core/src/test/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactoryTest.java @@ -1629,7 +1629,82 @@ public class AffinityPlacementFactoryTest extends AbstractPlacementFactoryTest { // Each expected placement is represented as a string "col shard replica-type fromNode -> // toNode" - Set<String> expectedPlacements = Set.of("b 1 TLOG 0 -> 2", "b 1 NRT 3 -> 4"); + // The duplicate replica of shard 1 of collection "a" is first moved off of node 0, then the + // regular weight-based balancing evens out the remaining core counts + Set<String> expectedPlacements = Set.of("a 1 NRT 0 -> 4", "a 2 TLOG 3 -> 2"); + verifyBalancing( + expectedPlacements, + balancePlan, + collectionBuilder.getShardBuilders(), + clusterBuilder.buildLiveNodes()); + } + + @Test + public void testBalancingAvoidMultiReplicaOnNode() throws Exception { + // Cluster nodes and their attributes + Builders.ClusterBuilder clusterBuilder = Builders.newClusterBuilder().initializeLiveNodes(4); + List<Builders.NodeBuilder> nodeBuilders = clusterBuilder.getLiveNodeBuilders(); + + // The collection already exists with shards and replicas + Builders.CollectionBuilder collectionBuilder = Builders.newCollectionBuilder("a"); + // Note that the collection as defined below is in a state that would NOT be returned by the + // placement plugin: shard 1 has two replicas on node 0 and two replicas on node 2. + // The plugin should still be able to place additional replicas as long as they don't break the + // rules. + List<List<String>> shardsReplicas = List.of(List.of("NRT 0", "NRT 0", "NRT 2", "NRT 2")); + collectionBuilder.customCollectionSetup(shardsReplicas, nodeBuilders); + clusterBuilder.addCollection(collectionBuilder); + + // Add another collection. Note that this is also unbalanced with two replicas on node 1 and two + // replicas on node 3. + // The intent of this test is to demonstrate that the plugin can move replicas so that + // a given node does not have more than one replica of a given shard. + collectionBuilder = Builders.newCollectionBuilder("b"); + shardsReplicas = List.of(List.of("NRT 1", "NRT 1", "NRT 3", "NRT 3")); + collectionBuilder.customCollectionSetup(shardsReplicas, nodeBuilders); + clusterBuilder.addCollection(collectionBuilder); + + BalanceRequestImpl balanceRequest = + new BalanceRequestImpl(new HashSet<>(clusterBuilder.buildLiveNodes())); + BalancePlan balancePlan = + plugin.computeBalancing(balanceRequest, clusterBuilder.buildPlacementContext()); + + // Each expected placement is represented as a string "col shard replica-type fromNode -> + // toNode" + Set<String> expectedPlacements = + Set.of("a 1 NRT 0 -> 1", "a 1 NRT 2 -> 3", "b 1 NRT 3 -> 2", "b 1 NRT 1 -> 0"); + verifyBalancing( + expectedPlacements, + balancePlan, + collectionBuilder.getShardBuilders(), + clusterBuilder.buildLiveNodes()); + } + + @Test + public void testBalancingAvoidMultiReplicaOnNodeAcrossAZs() throws Exception { + // Cluster with two nodes in each of two availability zones + Builders.ClusterBuilder clusterBuilder = Builders.newClusterBuilder().initializeLiveNodes(4); + List<Builders.NodeBuilder> nodeBuilders = clusterBuilder.getLiveNodeBuilders(); + for (int i = 0; i < nodeBuilders.size(); i++) { + nodeBuilders + .get(i) + .setSysprop(AffinityPlacementConfig.AVAILABILITY_ZONE_SYSPROP, i < 2 ? "az1" : "az2"); + } + + // Shard 1 has both its replicas on node 0 in az1 + Builders.CollectionBuilder collectionBuilder = Builders.newCollectionBuilder("a"); + List<List<String>> shardsReplicas = List.of(List.of("NRT 0", "NRT 0")); + collectionBuilder.customCollectionSetup(shardsReplicas, nodeBuilders); + clusterBuilder.addCollection(collectionBuilder); + + BalanceRequestImpl balanceRequest = + new BalanceRequestImpl(new HashSet<>(clusterBuilder.buildLiveNodes())); + BalancePlan balancePlan = + plugin.computeBalancing(balanceRequest, clusterBuilder.buildPlacementContext()); + + // The duplicate replica must move to a node in az2 to keep the availability zones balanced, + // not to the empty az1 node 1 + Set<String> expectedPlacements = Set.of("a 1 NRT 0 -> 2"); verifyBalancing( expectedPlacements, balancePlan, @@ -1668,7 +1743,9 @@ public class AffinityPlacementFactoryTest extends AbstractPlacementFactoryTest { // Each expected placement is represented as a string "col shard replica-type fromNode -> // toNode" - Set<String> expectedPlacements = Set.of("a 1 NRT 3 -> 1", "a 2 NRT 3 -> 0"); + // The duplicate replica of shard 1 is first moved off of node 0, then the regular weight-based + // balancing evens out the remaining core counts + Set<String> expectedPlacements = Set.of("a 1 NRT 0 -> 1", "a 1 NRT 3 -> 2", "a 2 NRT 3 -> 0"); verifyBalancing( expectedPlacements, balancePlan, diff --git a/solr/core/src/test/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactoryTest.java b/solr/core/src/test/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactoryTest.java index e4cb6e71a94..a8cbeddda94 100644 --- a/solr/core/src/test/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactoryTest.java +++ b/solr/core/src/test/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactoryTest.java @@ -257,7 +257,9 @@ public class MinimizeCoresPlacementFactoryTest extends AbstractPlacementFactoryT // Each expected placement is represented as a string "col shard replica-type fromNode -> // toNode" - Set<String> expectedPlacements = Set.of("b 1 TLOG 0 -> 2", "b 1 NRT 3 -> 4"); + // The duplicate replica of shard 1 of collection "a" is first moved off of node 0, then the + // regular weight-based balancing evens out the remaining core counts + Set<String> expectedPlacements = Set.of("a 1 NRT 0 -> 4", "a 2 TLOG 3 -> 2"); verifyBalancing( expectedPlacements, balancePlan, @@ -296,7 +298,9 @@ public class MinimizeCoresPlacementFactoryTest extends AbstractPlacementFactoryT // Each expected placement is represented as a string "col shard replica-type fromNode -> // toNode" - Set<String> expectedPlacements = Set.of("a 1 NRT 3 -> 1", "a 2 NRT 3 -> 0"); + // The duplicate replica of shard 1 is first moved off of node 0, then the regular weight-based + // balancing evens out the remaining core counts + Set<String> expectedPlacements = Set.of("a 1 NRT 0 -> 1", "a 1 NRT 3 -> 2", "a 2 NRT 3 -> 0"); verifyBalancing( expectedPlacements, balancePlan,
