This is an automated email from the ASF dual-hosted git repository.

houston pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git


The following commit(s) were added to refs/heads/main by this push:
     new d5b9908  SOLR-15209: Make the LegacyAssignStrategy the 
LegacyPlacementPlugin (#512)
d5b9908 is described below

commit d5b99086daf1d2ad406ea98e97814d8dcc2e4c46
Author: Houston Putman <[email protected]>
AuthorDate: Thu Jan 13 17:32:13 2022 -0500

    SOLR-15209: Make the LegacyAssignStrategy the LegacyPlacementPlugin (#512)
---
 solr/CHANGES.txt                                   |   3 +
 .../apache/solr/cloud/api/collections/Assign.java  | 136 ++-----------------
 .../cluster/placement/PlacementPluginFactory.java  |   5 +-
 .../placement/plugins/LegacyPlacementFactory.java  | 149 +++++++++++++++++++++
 .../plugins/MinimizeCoresPlacementFactory.java     |   2 +-
 .../placement/plugins/RandomPlacementFactory.java  |   4 +-
 .../OverseerCollectionConfigSetProcessorTest.java  |  19 +--
 .../solr/servlet/HttpSolrCallGetCoreTest.java      |   1 -
 .../src/replica-placement-plugins.adoc             |  23 ++--
 9 files changed, 189 insertions(+), 153 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 9b94c44..901349a 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -226,6 +226,9 @@ when told to. The admin UI now tells it to. (Nazerke 
Seidan, David Smiley)
 
 * SOLR-14608: Faster sorting for the /export handler (Joel Bernstein)
 
+* SOLR-15209: The old LegacyAssignStrategy has been refactored into the 
LegacyPlacementPlugin. This is still the default
+  placement policy for Solr. (Houston Putman, Ilan Ginzburg)
+
 Build
 ---------------------
 
diff --git 
a/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java 
b/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
index 664bc50..02a573c 100644
--- a/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
+++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
@@ -22,7 +22,6 @@ import static 
org.apache.solr.common.cloud.ZkStateReader.CORE_NAME_PROP;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.util.*;
-import java.util.stream.Collectors;
 
 import org.apache.solr.client.solrj.cloud.DistribStateManager;
 import org.apache.solr.client.solrj.cloud.SolrCloudManager;
@@ -31,6 +30,7 @@ import org.apache.solr.client.solrj.cloud.BadVersionException;
 import org.apache.solr.client.solrj.cloud.VersionedData;
 import org.apache.solr.cluster.placement.PlacementPlugin;
 import org.apache.solr.cluster.placement.impl.PlacementPluginAssignStrategy;
+import org.apache.solr.cluster.placement.plugins.LegacyPlacementFactory;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.DocCollection;
@@ -49,8 +49,6 @@ import org.apache.zookeeper.KeeperException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.collect.ImmutableMap;
-
 public class Assign {
   private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
@@ -257,24 +255,7 @@ public class Assign {
     }
   }
 
-  static class ReplicaCount {
-    public final String nodeName;
-    public Map<String, Integer> collectionReplicas;
-    public int totalReplicas = 0;
-
-    ReplicaCount(String nodeName) {
-      this.nodeName = nodeName;
-      this.collectionReplicas = new HashMap<>();
-    }
-
-    public int weight(String collection) {
-      return (collectionReplicas.getOrDefault(collection, 0) * 100) + 
totalReplicas;
-    }
 
-    public String nodeName() {
-      return nodeName;
-    }
-  }
 
   // Only called from addReplica (and by extension createShard) (so far).
   //
@@ -313,32 +294,6 @@ public class Assign {
     return assignStrategy.assign(cloudManager, assignRequest);
   }
 
-  static void addNodeNameVsShardCount(ClusterState clusterState, 
HashMap<String, ReplicaCount> nodeNameVsShardCount) {
-    Collection<String> liveNodes = clusterState.getLiveNodes();
-
-    for (String s : liveNodes) {
-      nodeNameVsShardCount.putIfAbsent(s, new ReplicaCount(s));
-    }
-
-    // if we get here we were not given a createNodeList, build a map with 
real counts.
-    Map<String, DocCollection> collections = clusterState.getCollectionsMap();
-    for (Map.Entry<String, DocCollection> entry : collections.entrySet()) {
-      DocCollection c = entry.getValue();
-      //identify suitable nodes  by checking the no:of cores in each of them
-      for (Slice slice : c.getSlices()) {
-        Collection<Replica> replicas = slice.getReplicas();
-        for (Replica replica : replicas) {
-          ReplicaCount count = nodeNameVsShardCount.get(replica.getNodeName());
-          if (count != null) {
-            // Used to "weigh" whether this node should be used later.
-            count.collectionReplicas.merge(entry.getKey(), 1, Integer::sum);
-            count.totalReplicas++;
-          }
-        }
-      }
-    }
-  }
-
   // throw an exception if any node in the supplied list is not live.
   // Empty or null list always succeeds and returns the input.
   private static List<String> checkLiveNodes(List<String> createNodeList, 
ClusterState clusterState) {
@@ -355,26 +310,6 @@ public class Assign {
     return createNodeList; // unmodified, but return for inline use
   }
 
-  // throw an exception if all nodes in the supplied list are not live.
-  // Empty list will also fail.
-  // Returns the input
-  private static List<String> checkAnyLiveNodes(List<String> createNodeList, 
ClusterState clusterState) {
-    Set<String> liveNodes = clusterState.getLiveNodes();
-    if (createNodeList == null) {
-      createNodeList = Collections.emptyList();
-    }
-    boolean anyLiveNodes = false;
-    for (String node : createNodeList) {
-      anyLiveNodes |= liveNodes.contains(node);
-    }
-    if (!anyLiveNodes) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
-          "None of the node(s) specified " + createNodeList + " are currently 
active in "
-              + liveNodes + ", no action taken.");
-    }
-    return createNodeList; // unmodified, but return for inline use. Only 
modified if empty, and that will throw an error
-  }
-
   /**
    * Thrown if there is an exception while assigning nodes for replicas
    */
@@ -525,73 +460,20 @@ public class Assign {
     }
   }
 
-  public static class LegacyAssignStrategy implements AssignStrategy {
-    @Override
-    public List<ReplicaPosition> assign(SolrCloudManager solrCloudManager, 
List<AssignRequest> assignRequests) throws Assign.AssignmentException, 
IOException, InterruptedException {
-      ClusterState clusterState = 
solrCloudManager.getClusterStateProvider().getClusterState();
-
-      List<ReplicaPosition> result = new ArrayList<>();
-
-      HashMap<String, Assign.ReplicaCount> nodeNameVsShardCount = new 
HashMap<>();
-      addNodeNameVsShardCount(clusterState, nodeNameVsShardCount);
-      for (AssignRequest assignRequest : assignRequests) {
-        Collection<ReplicaCount> replicaCounts = nodeNameVsShardCount.values();
-
-        if (assignRequest.nodes != null && !assignRequest.nodes.isEmpty()) {
-          // Throw an error if there are any non-live nodes.
-          checkLiveNodes(assignRequest.nodes, clusterState);
-          HashSet<String> nodeSet = new HashSet<>(assignRequest.nodes);
-          replicaCounts = replicaCounts.stream().filter(rc -> 
nodeSet.contains(rc.nodeName)).collect(Collectors.toList());
-        } else if (nodeNameVsShardCount.values().isEmpty()) {
-          throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "There 
are no live nodes in the cluster");
-        }
-
-        for (String aShard : assignRequest.shardNames) {
-          // Reset the ordering of the nodes for each shard, using the 
replicas added in the previous shards and assign requests
-          List<String> nodeList = replicaCounts.stream()
-                  .sorted(Comparator.<ReplicaCount>comparingInt(rc -> 
rc.weight(assignRequest.collectionName)).thenComparing(ReplicaCount::nodeName))
-                  .map(ReplicaCount::nodeName)
-                  .collect(Collectors.toList());
-          int i = 0;
-          for (Map.Entry<Replica.Type, Integer> e : 
countsPerReplicaType(assignRequest).entrySet()) {
-            for (int j = 0; j < e.getValue(); j++) {
-              String assignedNode = nodeList.get(i % nodeList.size());
-              result.add(new ReplicaPosition(assignRequest.collectionName, 
aShard, j, e.getKey(), assignedNode));
-              i++;
-              ReplicaCount replicaCount = 
nodeNameVsShardCount.computeIfAbsent(assignedNode, ReplicaCount::new);
-              replicaCount.totalReplicas++;
-              
replicaCount.collectionReplicas.merge(assignRequest.collectionName, 1, 
Integer::sum);
-            }
-          }
-        }
-      }
-
-      return result;
-    }
-
-    // keeps this big ugly construction block out of otherwise legible code
-    private ImmutableMap<Replica.Type, Integer> 
countsPerReplicaType(AssignRequest assignRequest) {
-      return ImmutableMap.of(
-          Replica.Type.NRT, assignRequest.numNrtReplicas,
-          Replica.Type.TLOG, assignRequest.numTlogReplicas,
-          Replica.Type.PULL, assignRequest.numPullReplicas
-      );
-    }
-  }
-
   /**
    * Creates the appropriate instance of {@link AssignStrategy} based on how 
the cluster and/or individual collections are
    * configured.
-   * <p>If {@link PlacementPlugin} instance is null this call will return 
{@link LegacyAssignStrategy}, otherwise
+   * <p>If {@link PlacementPlugin} instance is null this call will return a 
strategy from {@link LegacyPlacementFactory}, otherwise
    * {@link PlacementPluginAssignStrategy} will be used.</p>
    */
   public static AssignStrategy createAssignStrategy(CoreContainer 
coreContainer) {
+    // If a cluster wide placement plugin is configured (and that's the only 
way to define a placement plugin)
     PlacementPlugin placementPlugin = 
coreContainer.getPlacementPluginFactory().createPluginInstance();
-    if (placementPlugin != null) {
-      // If a cluster wide placement plugin is configured (and that's the only 
way to define a placement plugin)
-      return new PlacementPluginAssignStrategy(placementPlugin);
-    }  else {
-        return new LegacyAssignStrategy();
-      }
+    if (placementPlugin == null) {
+      // Otherwise use the default
+      // TODO: Replace this with a better options, such as the 
AffinityPlacementFactory
+      placementPlugin = (new LegacyPlacementFactory()).createPluginInstance();
     }
+    return new PlacementPluginAssignStrategy(placementPlugin);
+  }
 }
diff --git 
a/solr/core/src/java/org/apache/solr/cluster/placement/PlacementPluginFactory.java
 
b/solr/core/src/java/org/apache/solr/cluster/placement/PlacementPluginFactory.java
index 3bfc0d8..138aab2 100644
--- 
a/solr/core/src/java/org/apache/solr/cluster/placement/PlacementPluginFactory.java
+++ 
b/solr/core/src/java/org/apache/solr/cluster/placement/PlacementPluginFactory.java
@@ -18,6 +18,7 @@
 package org.apache.solr.cluster.placement;
 
 import org.apache.solr.api.ConfigurablePlugin;
+import org.apache.solr.cluster.placement.plugins.LegacyPlacementFactory;
 
 /**
  * Factory implemented by client code and configured in container plugins
@@ -38,8 +39,8 @@ public interface PlacementPluginFactory<T extends 
PlacementPluginConfig> extends
    * Returns an instance of the plugin that will be repeatedly (and 
concurrently) called to compute placement. Multiple
    * instances of a plugin can be used in parallel (for example if 
configuration has to change, but plugin instances with
    * the previous configuration are still being used).
-   * <p>If this method returns null then a simple legacy assignment strategy 
will be used
-   * (see {@link 
org.apache.solr.cloud.api.collections.Assign.LegacyAssignStrategy}).</p>
+   * <p>If this method returns null then a simple default assignment strategy 
will be used
+   * (see {@link LegacyPlacementFactory}).</p>
    */
   PlacementPlugin createPluginInstance();
 
diff --git 
a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/LegacyPlacementFactory.java
 
b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/LegacyPlacementFactory.java
new file mode 100644
index 0000000..c14a737
--- /dev/null
+++ 
b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/LegacyPlacementFactory.java
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.cluster.placement.plugins;
+
+import org.apache.solr.cluster.Node;
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.PlacementContext;
+import org.apache.solr.cluster.placement.PlacementException;
+import org.apache.solr.cluster.placement.PlacementPlan;
+import org.apache.solr.cluster.placement.PlacementPlugin;
+import org.apache.solr.cluster.placement.PlacementPluginFactory;
+import org.apache.solr.cluster.placement.PlacementRequest;
+import org.apache.solr.cluster.placement.ReplicaPlacement;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * <p>Factory for creating {@link LegacyPlacementPlugin}, a placement plugin 
implementing the logic from the old <code>LegacyAssignStrategy</code>.
+ *  This chooses nodes with the fewest cores (especially cores of the same 
collection).</p>
+ *
+ * <p>See {@link AffinityPlacementFactory} for a more realistic example and 
documentation.</p>
+ */
+public class LegacyPlacementFactory implements 
PlacementPluginFactory<PlacementPluginFactory.NoConfig> {
+
+  @Override
+  public PlacementPlugin createPluginInstance() {
+    return new LegacyPlacementPlugin();
+  }
+
+  public static class LegacyPlacementPlugin implements PlacementPlugin {
+    @Override
+    public List<PlacementPlan> computePlacements(Collection<PlacementRequest> 
requests, PlacementContext placementContext) throws PlacementException {
+      List<PlacementPlan> placementPlans = new ArrayList<>(requests.size());
+      Map<Node, ReplicaCount> nodeVsShardCount = 
getNodeVsShardCount(placementContext);
+      for (PlacementRequest request : requests) {
+        int totalReplicasPerShard = 0;
+        for (Replica.ReplicaType rt : Replica.ReplicaType.values()) {
+          totalReplicasPerShard += request.getCountReplicasToCreate(rt);
+        }
+
+        Set<ReplicaPlacement> replicaPlacements = new 
HashSet<>(totalReplicasPerShard * request.getShardNames().size());
+
+        Collection<ReplicaCount> replicaCounts = nodeVsShardCount.values();
+
+        if (request.getTargetNodes().size() < replicaCounts.size()) {
+          replicaCounts = replicaCounts.stream().filter(rc -> 
request.getTargetNodes().contains(rc.node())).collect(Collectors.toList());
+        }
+
+        for (String shard : request.getShardNames()) {
+          // Reset the ordering of the nodes for each shard, using the 
replicas added in the previous shards and assign requests
+          List<Node> nodeList = replicaCounts.stream()
+              .sorted(Comparator.<ReplicaCount>comparingInt(rc -> 
rc.weight(request.getCollection().getName())).thenComparing(ReplicaCount::nodeName))
+              .map(ReplicaCount::node)
+              .collect(Collectors.toList());
+          int replicaNumOfShard = 0;
+          for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) 
{
+            for (int i = 0; i < request.getCountReplicasToCreate(replicaType); 
i++) {
+              Node assignedNode = nodeList.get(replicaNumOfShard++ % 
nodeList.size());
+
+              
replicaPlacements.add(placementContext.getPlacementPlanFactory().createReplicaPlacement(request.getCollection(),
 shard, assignedNode, replicaType));
+
+              ReplicaCount replicaCount = 
nodeVsShardCount.computeIfAbsent(assignedNode, ReplicaCount::new);
+              replicaCount.totalReplicas++;
+              
replicaCount.collectionReplicas.merge(request.getCollection().getName(), 1, 
Integer::sum);
+            }
+          }
+        }
+
+        
placementPlans.add(placementContext.getPlacementPlanFactory().createPlacementPlan(request,
 replicaPlacements));
+      }
+      return placementPlans;
+    }
+
+    private Map<Node, ReplicaCount> getNodeVsShardCount(PlacementContext 
placementContext) {
+      HashMap<Node, ReplicaCount> nodeVsShardCount = new HashMap<>();
+
+      for (Node s : placementContext.getCluster().getLiveDataNodes()) {
+        nodeVsShardCount.computeIfAbsent(s, ReplicaCount::new);
+      }
+
+      // if we get here we were not given a createNodeList, build a map with 
real counts.
+      for (SolrCollection collection : 
placementContext.getCluster().collections()) {
+        //identify suitable nodes  by checking the no:of cores in each of them
+        for (Shard shard : collection.shards()) {
+          for (Replica replica : shard.replicas()) {
+            ReplicaCount count = nodeVsShardCount.get(replica.getNode());
+            if (count != null) {
+              count.addReplica(collection.getName(), shard.getShardName());
+            }
+          }
+        }
+      }
+      return nodeVsShardCount;
+    }
+  }
+
+  static class ReplicaCount {
+    public final Node node;
+    public Map<String, Integer> collectionReplicas;
+    public int totalReplicas = 0;
+
+    ReplicaCount(Node node) {
+      this.node = node;
+      this.collectionReplicas = new HashMap<>();
+    }
+
+    public int weight(String collection) {
+      return (collectionReplicas.getOrDefault(collection, 0) * 5) + 
totalReplicas;
+    }
+
+    public void addReplica(String collection, String shard) {
+      // Used to "weigh" whether this node should be used later.
+      collectionReplicas.merge(collection, 1, Integer::sum);
+    }
+
+    public Node node() {
+      return node;
+    }
+
+    public String nodeName() {
+      return node.getName();
+    }
+  }
+}
\ No newline at end of file
diff --git 
a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactory.java
 
b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactory.java
index ddd327f..a36a881 100644
--- 
a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactory.java
+++ 
b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactory.java
@@ -80,7 +80,7 @@ public class MinimizeCoresPlacementFactory implements 
PlacementPluginFactory<Pla
           totalReplicasPerShard += request.getCountReplicasToCreate(rt);
         }
 
-        if (placementContext.getCluster().getLiveNodes().size() < 
totalReplicasPerShard) {
+        if (request.getTargetNodes().size() < totalReplicasPerShard) {
           throw new PlacementException("Cluster size too small for number of 
replicas per shard");
         }
 
diff --git 
a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/RandomPlacementFactory.java
 
b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/RandomPlacementFactory.java
index 2f03b5f..e724ab7 100644
--- 
a/solr/core/src/java/org/apache/solr/cluster/placement/plugins/RandomPlacementFactory.java
+++ 
b/solr/core/src/java/org/apache/solr/cluster/placement/plugins/RandomPlacementFactory.java
@@ -63,7 +63,7 @@ public class RandomPlacementFactory implements 
PlacementPluginFactory<PlacementP
           totalReplicasPerShard += request.getCountReplicasToCreate(rt);
         }
 
-        if (placementContext.getCluster().getLiveNodes().size() < 
totalReplicasPerShard) {
+        if (request.getTargetNodes().size() < totalReplicasPerShard) {
           throw new PlacementException("Cluster size too small for number of 
replicas per shard");
         }
 
@@ -72,7 +72,7 @@ public class RandomPlacementFactory implements 
PlacementPluginFactory<PlacementP
         // Now place randomly all replicas of all shards on available nodes
         for (String shardName : request.getShardNames()) {
           // Shuffle the nodes for each shard so that replicas for a shard are 
placed on distinct yet random nodes
-          ArrayList<Node> nodesToAssign = new 
ArrayList<>(placementContext.getCluster().getLiveNodes());
+          ArrayList<Node> nodesToAssign = new 
ArrayList<>(request.getTargetNodes());
           Collections.shuffle(nodesToAssign, replicaPlacementRandom);
 
           for (Replica.ReplicaType rt : Replica.ReplicaType.values()) {
diff --git 
a/solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java
 
b/solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java
index 86e9ca6..0059504 100644
--- 
a/solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java
+++ 
b/solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java
@@ -318,7 +318,8 @@ public class OverseerCollectionConfigSetProcessorTest 
extends SolrTestCaseJ4 {
       return null;
     }).when(zkStateReaderMock).waitForState(anyString(), anyLong(), any(), 
any(Predicate.class));
 
-    when(clusterStateMock.getCollection(anyString())).thenAnswer(invocation -> 
{
+    when(clusterStateMock.getCollection(anyString())).thenCallRealMethod();
+    
when(clusterStateMock.getCollectionOrNull(anyString())).thenAnswer(invocation 
-> {
       String key = invocation.getArgument(0);
       if (!collectionsSet.containsKey(key)) return null;
       DocCollection docCollection = collectionsSet.get(key).get();
@@ -633,6 +634,7 @@ public class OverseerCollectionConfigSetProcessorTest 
extends SolrTestCaseJ4 {
           .substring(7);
       nodeUrlWithoutProtocolPartForLiveNodes.add(nodeUrlWithoutProtocolPart);
     }
+    final Map<String,Set<String>> shard_TO_coreNames_map = new HashMap<>();
     final Map<String,String> 
coreName_TO_nodeUrlWithoutProtocolPartForLiveNodes_map = new HashMap<>();
 
     ArgumentCaptor<ShardRequest> shardRequestCaptor = 
ArgumentCaptor.forClass(ShardRequest.class);
@@ -649,9 +651,8 @@ public class OverseerCollectionConfigSetProcessorTest 
extends SolrTestCaseJ4 {
           shardRequest.params.get(CoreAdminParams.ACTION));
       // assertEquals(shardRequest.params, submitCapture.params);
       String coreName = shardRequest.params.get(CoreAdminParams.NAME);
-      assertFalse("Core with name " + coreName + " created twice",
-          coreNames.contains(coreName));
-      coreNames.add(coreName);
+      assertTrue("Core with name " + coreName + " created twice", 
coreNames.add(coreName));
+      
shard_TO_coreNames_map.computeIfAbsent(shardRequest.params.get(CoreAdminParams.SHARD),
 shard -> new HashSet<>()).add(coreName);
       assertEquals(CONFIG_NAME,
           shardRequest.params.get("collection.configName"));
       assertEquals(COLLECTION_NAME,
@@ -674,7 +675,7 @@ public class OverseerCollectionConfigSetProcessorTest 
extends SolrTestCaseJ4 {
       if (!sliceToNodeUrlsWithoutProtocolPartToNumberOfShardsRunningMapMap
           .containsKey(sliceName)) {
         sliceToNodeUrlsWithoutProtocolPartToNumberOfShardsRunningMapMap.put(
-            sliceName, new HashMap<String,Integer>());
+            sliceName, new HashMap<>());
       }
       Map<String,Integer> 
nodeUrlsWithoutProtocolPartToNumberOfShardsRunningMap = 
sliceToNodeUrlsWithoutProtocolPartToNumberOfShardsRunningMapMap
           .get(sliceName);
@@ -688,11 +689,11 @@ public class OverseerCollectionConfigSetProcessorTest 
extends SolrTestCaseJ4 {
     }
     
     assertEquals(numberOfSlices * numberOfReplica, coreNames.size());
-    for (int i = 1; i <= numberOfSlices; i++) {
+    assertEquals("Wrong number of shards", numberOfSlices.intValue(), 
shard_TO_coreNames_map.size());
+    for (Map.Entry<String, Set<String>> entry : 
shard_TO_coreNames_map.entrySet()) {
+      assertEquals("Wrong number of cores for shard " + entry.getKey(), 
numberOfReplica.intValue(), entry.getValue().size());
       Set<String> foundNodeNames = new HashSet<>(numberOfReplica);
-      for (int j = 1; j <= numberOfReplica; j++) {
-        String coreName = coreNames.get((i-1) * numberOfReplica + (j-1));
-
+      for (String coreName : entry.getValue()) {
         String foundNode = 
coreName_TO_nodeUrlWithoutProtocolPartForLiveNodes_map.get(coreName);
         assertTrue("Multiple replicas scheduled for node: "+foundNode, 
foundNodeNames.add(foundNode));
         assertTrue("Assigned node name not in list of given nodes: 
"+foundNode, nodeUrlWithoutProtocolPartForLiveNodes.contains(foundNode));
diff --git 
a/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallGetCoreTest.java 
b/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallGetCoreTest.java
index 7c06028..22fda8d 100644
--- a/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallGetCoreTest.java
+++ b/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallGetCoreTest.java
@@ -35,7 +35,6 @@ import org.eclipse.jetty.server.Response;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
-// commented 4-Sep-2018 
@LuceneTestCase.BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028";)
 // 2-Aug-2018
 public class HttpSolrCallGetCoreTest extends SolrCloudTestCase {
   private static final String COLLECTION = "collection1";
   private static final int NUM_SHARD = 3;
diff --git a/solr/solr-ref-guide/src/replica-placement-plugins.adoc 
b/solr/solr-ref-guide/src/replica-placement-plugins.adoc
index 84d0e75..d1ea48c 100644
--- a/solr/solr-ref-guide/src/replica-placement-plugins.adoc
+++ b/solr/solr-ref-guide/src/replica-placement-plugins.adoc
@@ -32,18 +32,9 @@ There are several placement plugins included in the Solr 
distribution.
 Additional placement plugins can be added - they have to implement a 
`PlacementPluginFactory` interface.
 The configuration entry may also contain a `config` element if the plugin 
implements the `ConfigurablePlugin` interface.
 
-== Legacy Replica Placement
-By default Solr 9.0 uses a legacy method for replica placements, which doesn't 
use a placement plugin at all.
-This method is used whenever the placement plugin configuration is missing or 
invalid.
-
-Legacy placement simply assigns new replicas to live nodes in a round-robin 
fashion: first it prepares a sorted list of nodes with the smallest number of 
existing replicas of the collection.
-Then for each shard in the request it adds the replicas to consecutive nodes 
in this order, wrapping around to the first node if the number of replicas is 
larger than the number of nodes.
-
-This placement strategy doesn't ensure that no more than 1 replica of a shard 
is placed on the same node.
-Also, the round-robin assignment only roughly approximates an even spread of 
replicas across the nodes.
-
 == Placement Plugins Included with Solr
-The following placement plugins are available out-of-the-box in Solr 9.0 
(however, as described above the default configuration doesn't use any of them, 
instead it uses the legacy replica placement method).
+The following placement plugins are available out-of-the-box in Solr 9.0.
+If no placement plugin is defined, Solr will default to using the 
<<#legacyplacementfactory,LegacyPlacementPlugin>>
 
 In order to use a plugin its configuration must be added using the 
`/cluster/plugin` API.
 For example, in order to use the `AffinityPlacementFactory` plugin the 
following command should be executed:
@@ -75,6 +66,16 @@ Similarly, the configuration can be updated or removed.
 NOTE: Placement plugin configuration MUST use the predefined name 
`.placement-plugin`.
 There can be only one (or none) placement configuration defined.
 
+=== LegacyPlacementFactory
+By default Solr 9.0 uses a legacy method for replica placements, the 
`LegacyPlacementFactory`.
+This method is used whenever the placement plugin configuration is missing or 
invalid.
+
+Legacy placement simply assigns new replicas to live nodes in a round-robin 
fashion: first it prepares a sorted list of nodes with the smallest number of 
existing replicas, especially of the collection.
+Then for each shard in the request it adds the replicas to consecutive nodes 
in this order, wrapping around to the first node if the number of replicas is 
larger than the number of nodes.
+
+This placement strategy doesn't ensure that no more than 1 replica of a shard 
is placed on the same node.
+Also, the round-robin assignment only roughly approximates an even spread of 
replicas across the nodes.
+
 === RandomPlacementFactory
 This plugin creates random placements, while preventing two replicas of the 
same shard from being placed on the same node.
 If there are too few nodes to satisfy these constraints an exception is 
thrown, and the request is rejected.

Reply via email to