murblanc commented on code in PR #2039:
URL: https://github.com/apache/solr/pull/2039#discussion_r1408434915


##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java:
##########
@@ -97,9 +94,12 @@
 public class CreateCollectionCmd implements CollApiCmds.CollectionApiCommand {
   private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
   private final CollectionCommandContext ccc;
+  private final EnumSet<Replica.Type> leaderEligibleReplicaTypes;
 
   public CreateCollectionCmd(CollectionCommandContext ccc) {
     this.ccc = ccc;
+    leaderEligibleReplicaTypes = 
CollectionHandlingUtils.leaderEligibleReplicaTypes();
+    ;

Review Comment:
   spare semicolon



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -135,6 +140,20 @@ public class CollectionHandlingUtils {
     }
   }
 
+  /** Returns names of properties that are used to specify a number of 
replicas of a given type. */

Review Comment:
   Wouldn't these two methods `numReplicasProperties` and 
`leaderEligibleReplicaTypes` belong in `Replica.Type`?



##########
solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java:
##########
@@ -103,31 +104,41 @@ public enum Type {
      * support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can 
become a leader. A shard
      * leader will forward updates to all active {@link Type#NRT} and {@link 
Type#TLOG} replicas.
      */
-    NRT(true),
+    NRT(true, CollectionAdminParams.NRT_REPLICAS),
     /**
      * Writes to transaction log, but not to index, uses replication. Any 
{@link Type#TLOG} replica
      * can become leader (by first applying all local transaction log 
elements). If a replica is of
      * type {@link Type#TLOG} but is also the leader, it will behave as a 
{@link Type#NRT}. A shard
      * leader will forward updates to all active {@link Type#NRT} and {@link 
Type#TLOG} replicas.
      */
-    TLOG(true),
+    TLOG(true, CollectionAdminParams.TLOG_REPLICAS),
     /**
      * Doesn’t index or writes to transaction log. Just replicates from {@link 
Type#NRT} or {@link
      * Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard 
leaders (i.e., if there
      * are only pull replicas in the collection at some point, updates will 
fail same as if there is
      * no leaders, queries continue to work), so they don’t even participate 
in elections.
      */
-    PULL(false);
+    PULL(false, CollectionAdminParams.PULL_REPLICAS);
 
     public final boolean leaderEligible;
+    public final String numReplicasProperty;

Review Comment:
   I suggest renaming this into `numReplicasPropertyName`



##########
solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java:
##########
@@ -103,31 +104,41 @@ public enum Type {
      * support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can 
become a leader. A shard
      * leader will forward updates to all active {@link Type#NRT} and {@link 
Type#TLOG} replicas.
      */
-    NRT(true),
+    NRT(true, CollectionAdminParams.NRT_REPLICAS),
     /**
      * Writes to transaction log, but not to index, uses replication. Any 
{@link Type#TLOG} replica
      * can become leader (by first applying all local transaction log 
elements). If a replica is of
      * type {@link Type#TLOG} but is also the leader, it will behave as a 
{@link Type#NRT}. A shard
      * leader will forward updates to all active {@link Type#NRT} and {@link 
Type#TLOG} replicas.
      */
-    TLOG(true),
+    TLOG(true, CollectionAdminParams.TLOG_REPLICAS),
     /**
      * Doesn’t index or writes to transaction log. Just replicates from {@link 
Type#NRT} or {@link
      * Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard 
leaders (i.e., if there
      * are only pull replicas in the collection at some point, updates will 
fail same as if there is
      * no leaders, queries continue to work), so they don’t even participate 
in elections.
      */
-    PULL(false);
+    PULL(false, CollectionAdminParams.PULL_REPLICAS);
 
     public final boolean leaderEligible;

Review Comment:
   Shall we add a `defaultReplica` final boolean here and set it to true for 
the replica defined as default (via `defaultType()`). This will will clean up 
some code needing the default replica type (for example 
`CollectionHandlingUtils.makeCollectionPropsAndDefaults()`).



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java:
##########
@@ -97,33 +101,30 @@ public void call(ClusterState clusterState, ZkNodeProps 
message, NamedList<Objec
         CollectionHandlingUtils.waitForNewShard(collectionName, sliceName, 
ccc.getZkStateReader());
 
     String async = message.getStr(ASYNC);
-    ZkNodeProps addReplicasProps =
-        new ZkNodeProps(
+    Map<String, Object> addReplicasProps =
+        Utils.makeMap(
             COLLECTION_PROP,
-            collectionName,
+            (Object) collectionName,
             SHARD_ID_PROP,
             sliceName,
-            ZkStateReader.NRT_REPLICAS,
-            String.valueOf(numReplicas.get(Replica.Type.NRT)),
-            ZkStateReader.TLOG_REPLICAS,
-            String.valueOf(numReplicas.get(Replica.Type.TLOG)),
-            ZkStateReader.PULL_REPLICAS,
-            String.valueOf(numReplicas.get(Replica.Type.PULL)),
             CollectionHandlingUtils.CREATE_NODE_SET,
             message.getStr(CollectionHandlingUtils.CREATE_NODE_SET),
             CommonAdminParams.WAIT_FOR_FINAL_STATE,
             Boolean.toString(waitForFinalState));
+    for (Replica.Type replicaType : numReplicas.keySet()) {

Review Comment:
   Since `ReplicaCount` already has a `fromMessage` method (and `fromProps`), 
wouldn't it make sense to have it implement a `toMessage` (not sure about the 
name) method returning a property -> count map, and here we would simply 
`addAll` its entries to the map?
   
   Or, more realistically given we might try to build the message in a 
`ZkNodeProps` directly (see comment below), have the `toMessage` take as a 
parameter the target message and add the replica counts there.
   
   In other places in SolrCloud a `Map` is used though. Maybe add two methods 
(or more, after seeing other places in the code using other types, commented 
there as well) in `ReplicaCount`. The `Map` based one can do the actual work 
and the other ones call it.



##########
solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java:
##########
@@ -100,6 +166,10 @@ public int total() {
     return countByType.values().stream().reduce(Integer::sum).orElse(0);
   }
 
+  public int count(EnumSet<Replica.Type> replicaTypes) {

Review Comment:
   This method is called from two places, and both calls are to get the total 
of leader eligible replicas, which forces the caller to build the (enum) set 
first.
   I suggest specializing this method and renaming it `countLeaderEligible` or 
something along these lines to simplify the callers.
   We could even go simpler and have a boolean method `hasLeaderReplicas`. The 
caller would look even better.



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java:
##########
@@ -97,33 +101,30 @@ public void call(ClusterState clusterState, ZkNodeProps 
message, NamedList<Objec
         CollectionHandlingUtils.waitForNewShard(collectionName, sliceName, 
ccc.getZkStateReader());
 
     String async = message.getStr(ASYNC);
-    ZkNodeProps addReplicasProps =
-        new ZkNodeProps(
+    Map<String, Object> addReplicasProps =
+        Utils.makeMap(
             COLLECTION_PROP,
-            collectionName,
+            (Object) collectionName,

Review Comment:
   Compiler unhappy without this cast?



##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1055,7 +1051,7 @@ public Map<String, Object> execute(
             DocCollection.verifyProp(m, prop);
           }
           if (m.get(REPLICATION_FACTOR) != null) {
-            m.put(NRT_REPLICAS, m.get(REPLICATION_FACTOR));
+            m.put(Replica.Type.defaultType().numReplicasProperty, 
m.get(REPLICATION_FACTOR));

Review Comment:
   Unrelated: wonder if we should stop putting `REPLICATION_FACTOR` parameters 
but keep accepting them if present, to phase this parameter out in a couple of 
major releases...



##########
solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java:
##########
@@ -49,6 +49,8 @@ public interface CollectionAdminParams {
 
   String REPLICATION_FACTOR = "replicationFactor";
 
+  String REPLICA_TYPE = "type";

Review Comment:
   There is already a `REPLICA_TYPE` in `ZkStateReader`



##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java:
##########
@@ -129,17 +129,8 @@ public String getRequestType() {
   }
 
   private static void writeNumReplicas(ReplicaCount numReplicas, 
ModifiableSolrParams params) {
-    if (numReplicas.contains(Replica.Type.NRT)) {
-      params.add(
-          CollectionAdminParams.NRT_REPLICAS, 
String.valueOf(numReplicas.get(Replica.Type.NRT)));
-    }
-    if (numReplicas.contains(Replica.Type.TLOG)) {
-      params.add(
-          CollectionAdminParams.TLOG_REPLICAS, 
String.valueOf(numReplicas.get(Replica.Type.TLOG)));
-    }
-    if (numReplicas.contains(Replica.Type.PULL)) {
-      params.add(
-          CollectionAdminParams.PULL_REPLICAS, 
String.valueOf(numReplicas.get(Replica.Type.PULL)));
+    for (Replica.Type replicaType : numReplicas.keySet()) {

Review Comment:
   Maybe a third destination type for a `ReplicaCount.toMessage()`? 😢 
   
   Do we need to introduce a functional interface to be able to do add's on all 
these different types from a single method and avoid all these for loops 
sprinkled over the code?



##########
solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java:
##########
@@ -18,31 +18,92 @@
 
 import java.util.Arrays;
 import java.util.EnumMap;
+import java.util.EnumSet;
 import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
 import java.util.stream.Collectors;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.CollectionAdminParams;
 
 /** Tracks the number of replicas per replica type. This class is mutable. */
 public final class ReplicaCount {
   private final EnumMap<Replica.Type, Integer> countByType;
 
-  public ReplicaCount(Integer nrtReplicas, Integer tlogReplicas, Integer 
pullReplicas) {
+  private ReplicaCount() {
     this.countByType = new EnumMap<>(Replica.Type.class);
-    put(Replica.Type.NRT, nrtReplicas);
-    put(Replica.Type.TLOG, tlogReplicas);
-    put(Replica.Type.PULL, pullReplicas);
   }
 
   public static ReplicaCount empty() {
-    return new ReplicaCount(null, null, null);
+    return new ReplicaCount();
   }
 
   public static ReplicaCount of(Replica.Type type, Integer count) {
-    ReplicaCount replicaCount = empty();
+    ReplicaCount replicaCount = new ReplicaCount();
     replicaCount.put(type, count);
     return replicaCount;
   }
 
+  public static ReplicaCount of(Integer nrtReplicas, Integer tlogReplicas, 
Integer pullReplicas) {
+    ReplicaCount replicaCount = new ReplicaCount();
+    replicaCount.put(Replica.Type.NRT, nrtReplicas);
+    replicaCount.put(Replica.Type.TLOG, tlogReplicas);
+    replicaCount.put(Replica.Type.PULL, pullReplicas);
+    return replicaCount;
+  }
+
+  public static ReplicaCount fromMessage(ZkNodeProps message) {
+    return fromMessage(message, null);
+  }
+
+  public static ReplicaCount fromMessage(ZkNodeProps message, DocCollection 
collection) {
+    return fromMessage(message, collection, null);
+  }
+
+  public static ReplicaCount fromMessage(

Review Comment:
   Need Javadoc for this method, it is obscure enough.
   Also, need javadoc on `fromProps` and an explanation why it is much simpler.



##########
solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java:
##########
@@ -61,6 +122,11 @@ public boolean contains(Replica.Type type) {
     return countByType.containsKey(type);
   }
 
+  /** Returns the replica types for which a number of replicas was explicitely 
defined. */
+  public Set<Replica.Type> keySet() {

Review Comment:
   Do we still need this method if this class implements a method to directly 
count the number of leader replicas or if there's at least one leader replica? 
(see comment on `count()` below)
   (doing the review in GitHub not in the IDE so can't easily find all callers)



##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java:
##########
@@ -129,17 +129,8 @@ public String getRequestType() {
   }
 
   private static void writeNumReplicas(ReplicaCount numReplicas, 
ModifiableSolrParams params) {
-    if (numReplicas.contains(Replica.Type.NRT)) {
-      params.add(
-          CollectionAdminParams.NRT_REPLICAS, 
String.valueOf(numReplicas.get(Replica.Type.NRT)));
-    }
-    if (numReplicas.contains(Replica.Type.TLOG)) {
-      params.add(
-          CollectionAdminParams.TLOG_REPLICAS, 
String.valueOf(numReplicas.get(Replica.Type.TLOG)));
-    }
-    if (numReplicas.contains(Replica.Type.PULL)) {
-      params.add(
-          CollectionAdminParams.PULL_REPLICAS, 
String.valueOf(numReplicas.get(Replica.Type.PULL)));
+    for (Replica.Type replicaType : numReplicas.keySet()) {

Review Comment:
   Maybe a third destination type for a `ReplicaCount.toMessage()`? 😢 
   
   Do we need to introduce a functional interface to be able to do add's on all 
these different types from a single method and avoid all these for loops 
sprinkled over the code?



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java:
##########
@@ -103,17 +104,19 @@ public class ReindexCollectionCmd implements 
CollApiCmds.CollectionApiCommand {
   public static final String STATE = "state";
   public static final String PHASE = "phase";
 
-  private static final List<String> COLLECTION_PARAMS =
-      Arrays.asList(
-          ZkStateReader.CONFIGNAME_PROP,
-          ZkStateReader.NUM_SHARDS_PROP,
-          ZkStateReader.NRT_REPLICAS,
-          ZkStateReader.PULL_REPLICAS,
-          ZkStateReader.TLOG_REPLICAS,
-          ZkStateReader.REPLICATION_FACTOR,
-          "shards",
-          CollectionAdminParams.CREATE_NODE_SET_PARAM,
-          CollectionAdminParams.CREATE_NODE_SET_SHUFFLE_PARAM);
+  private static final List<String> COLLECTION_PARAMS = makeCollectionParams();
+
+  private static List<String> makeCollectionParams() {
+    List<String> collectionParams = new ArrayList<>(10);
+    collectionParams.add(ZkStateReader.CONFIGNAME_PROP);
+    collectionParams.add(ZkStateReader.NUM_SHARDS_PROP);
+    collectionParams.add(ZkStateReader.REPLICATION_FACTOR);
+    collectionParams.add("shards");
+    collectionParams.add(CollectionAdminParams.CREATE_NODE_SET_PARAM);
+    collectionParams.add(CollectionAdminParams.CREATE_NODE_SET_SHUFFLE_PARAM);
+    collectionParams.addAll(CollectionHandlingUtils.numReplicasProperties());
+    return List.copyOf(collectionParams);

Review Comment:
   Why copy instead of returning `collectionParams`?



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java:
##########
@@ -341,10 +318,10 @@ private void createCoreLessCollection(
       propMap.put(Overseer.QUEUE_OPERATION, CREATE.toString());
       // mostly true. Prevents autoCreated=true in the collection state.
       propMap.put("fromApi", "true");
-      propMap.put(REPLICATION_FACTOR, numReplicas.get(Replica.Type.NRT));
-      propMap.put(NRT_REPLICAS, numReplicas.get(Replica.Type.NRT));
-      propMap.put(TLOG_REPLICAS, numReplicas.get(Replica.Type.TLOG));
-      propMap.put(PULL_REPLICAS, numReplicas.get(Replica.Type.PULL));
+      propMap.put(REPLICATION_FACTOR, 
numReplicas.get(Replica.Type.defaultType()));
+      for (Replica.Type replicaType : numReplicas.keySet()) {

Review Comment:
   Here too we might be able to use here one of the ReplicaCount.toMessage 
methods?



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java:
##########
@@ -338,14 +339,8 @@ public void call(ClusterState clusterState, ZkNodeProps 
message, NamedList<Objec
       if (rf != null) {
         propMap.put(ZkStateReader.REPLICATION_FACTOR, rf);
       }
-      if (numNrt != null) {
-        propMap.put(ZkStateReader.NRT_REPLICAS, numNrt);
-      }
-      if (numTlog != null) {
-        propMap.put(ZkStateReader.TLOG_REPLICAS, numTlog);
-      }
-      if (numPull != null) {
-        propMap.put(ZkStateReader.PULL_REPLICAS, numPull);
+      for (Replica.Type replicaType : numReplicas.keySet()) {

Review Comment:
   Maybe we can use here one of the `ReplicaCount.toMessage` methods I suggest 
adding?



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java:
##########
@@ -590,12 +589,22 @@ public static List<String> populateShardNames(ZkNodeProps 
message, String router
     return shardNames;
   }
 
-  private static ReplicaCount getNumReplicas(ZkNodeProps message) {
-    int numTlogReplicas = message.getInt(TLOG_REPLICAS, 0);
-    int numNrtReplicas =
-        message.getInt(
-            NRT_REPLICAS, message.getInt(REPLICATION_FACTOR, numTlogReplicas > 
0 ? 0 : 1));
-    return new ReplicaCount(numNrtReplicas, numTlogReplicas, 
message.getInt(PULL_REPLICAS, 0));
+  private ReplicaCount getNumReplicas(ZkNodeProps message) {
+    ReplicaCount numReplicas = ReplicaCount.fromMessage(message);
+    int numLeaderEligibleReplicas = 
numReplicas.count(leaderEligibleReplicaTypes);
+    if (numLeaderEligibleReplicas == 0 && 
!numReplicas.contains(Replica.Type.defaultType())) {
+      // Ensure that there is at least one replica that can become leader if 
the user did
+      // not force a replica count.
+      numReplicas.put(Replica.Type.defaultType(), 1);
+    } else if (numLeaderEligibleReplicas == 0) {
+      // This can still fail if the user manually forced "0" replica counts.

Review Comment:
   We could simplify by removing the `else` branch here and keeping only the 
`numLeaderEligibleReplicas == 0` in the `if` condition, basically adding a 
default replica (assumed to be leader eligible) if the user passed 0 leaders in 
the request.
   
   BTW the `numReplicas.validate()` call in existing code (line 142) was 
useless since upon return from `getNumReplicas` there was always at least one 
leader replica.



##########
solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java:
##########
@@ -103,31 +104,41 @@ public enum Type {
      * support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can 
become a leader. A shard
      * leader will forward updates to all active {@link Type#NRT} and {@link 
Type#TLOG} replicas.
      */
-    NRT(true),
+    NRT(true, CollectionAdminParams.NRT_REPLICAS),
     /**
      * Writes to transaction log, but not to index, uses replication. Any 
{@link Type#TLOG} replica
      * can become leader (by first applying all local transaction log 
elements). If a replica is of
      * type {@link Type#TLOG} but is also the leader, it will behave as a 
{@link Type#NRT}. A shard
      * leader will forward updates to all active {@link Type#NRT} and {@link 
Type#TLOG} replicas.
      */
-    TLOG(true),
+    TLOG(true, CollectionAdminParams.TLOG_REPLICAS),
     /**
      * Doesn’t index or writes to transaction log. Just replicates from {@link 
Type#NRT} or {@link
      * Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard 
leaders (i.e., if there
      * are only pull replicas in the collection at some point, updates will 
fail same as if there is
      * no leaders, queries continue to work), so they don’t even participate 
in elections.
      */
-    PULL(false);
+    PULL(false, CollectionAdminParams.PULL_REPLICAS);
 
     public final boolean leaderEligible;
+    public final String numReplicasProperty;
 
-    Type(boolean b) {
-      this.leaderEligible = b;
+    Type(boolean leaderEligible, String numReplicasProperty) {
+      this.leaderEligible = leaderEligible;
+      this.numReplicasProperty = numReplicasProperty;
     }
 
     public static Type get(String name) {
       return StrUtils.isNullOrEmpty(name) ? NRT : 
Type.valueOf(name.toUpperCase(Locale.ROOT));
     }
+
+    /**
+     * Returns a default replica type. It is most notably used by the replica 
factor, which maps
+     * onto this replica type.

Review Comment:
   Maybe add a comment that this replica needs to be leader eligible



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java:
##########
@@ -97,33 +101,30 @@ public void call(ClusterState clusterState, ZkNodeProps 
message, NamedList<Objec
         CollectionHandlingUtils.waitForNewShard(collectionName, sliceName, 
ccc.getZkStateReader());
 
     String async = message.getStr(ASYNC);
-    ZkNodeProps addReplicasProps =
-        new ZkNodeProps(
+    Map<String, Object> addReplicasProps =
+        Utils.makeMap(
             COLLECTION_PROP,
-            collectionName,
+            (Object) collectionName,
             SHARD_ID_PROP,
             sliceName,
-            ZkStateReader.NRT_REPLICAS,
-            String.valueOf(numReplicas.get(Replica.Type.NRT)),
-            ZkStateReader.TLOG_REPLICAS,
-            String.valueOf(numReplicas.get(Replica.Type.TLOG)),
-            ZkStateReader.PULL_REPLICAS,
-            String.valueOf(numReplicas.get(Replica.Type.PULL)),
             CollectionHandlingUtils.CREATE_NODE_SET,
             message.getStr(CollectionHandlingUtils.CREATE_NODE_SET),
             CommonAdminParams.WAIT_FOR_FINAL_STATE,
             Boolean.toString(waitForFinalState));
+    for (Replica.Type replicaType : numReplicas.keySet()) {
+      addReplicasProps.put(replicaType.numReplicasProperty, 
numReplicas.get(replicaType));
+    }
 
-    Map<String, Object> propertyParams = new HashMap<>();
-    CollectionHandlingUtils.addPropertyParams(message, propertyParams);
-    addReplicasProps = addReplicasProps.plus(propertyParams);
-    if (async != null) addReplicasProps.getProperties().put(ASYNC, async);
+    CollectionHandlingUtils.addPropertyParams(message, addReplicasProps);
+    if (async != null) {
+      addReplicasProps.put(ASYNC, async);
+    }
     final NamedList<Object> addResult = new NamedList<>();
     try {
       new AddReplicaCmd(ccc)
           .addReplica(
               clusterState,
-              addReplicasProps,
+              new ZkNodeProps(addReplicasProps),

Review Comment:
   We should avoid copying maps if possible. Could we build the message in a 
`ZkNodeProps` like existing code does.



##########
solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java:
##########
@@ -70,15 +69,17 @@
 @Path("/backups/{backupName}/restore")
 public class RestoreCollectionAPI extends BackupAPIBase {
 
-  private static final Set<String> CREATE_PARAM_ALLOWLIST =
-      Set.of(
-          COLL_CONF,
-          REPLICATION_FACTOR,
-          NRT_REPLICAS,
-          TLOG_REPLICAS,
-          PULL_REPLICAS,
-          CREATE_NODE_SET_PARAM,
-          CREATE_NODE_SET_SHUFFLE_PARAM);
+  private static final Set<String> CREATE_PARAM_ALLOWLIST = 
Set.copyOf(makeCreateParamAllowList());

Review Comment:
   I wouldn't do a copy. I know in existing code `Set.of()` is unmodifiable, 
but `CREATE_PARAM_ALLOWLIST` is private and used in a single place.



##########
solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java:
##########
@@ -33,21 +33,6 @@ public class PlacementRequestImpl implements 
PlacementRequest {
   private final Set<Node> targetNodes;
   private final ReplicaCount numReplicas;
 
-  @Deprecated

Review Comment:
   Ok to remove since it's internal and not used prior to this PR, but it's not 
directly related to nor needed by this PR.



##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -262,12 +258,17 @@ public static Object verifyProp(Map<String, Object> 
props, String propName) {
 
   public static Object verifyProp(Map<String, Object> props, String propName, 
Object def) {
     Object o = props.get(propName);
-    if (o == null) return def;
+    if (o == null) {
+      return def;
+    }
+    // Properties associated with a number of replicas are parsed as integers.

Review Comment:
   Maybe put the replica type loop in the `default` case of the `switch`, so 
it's not run for no reason for each property? (I'm assuming the switch is 
optimized).



-- 
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: issues-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org

Reply via email to