review comments

apache · Oct 5, 2023 · 3ef90e0 · 3ef90e0
1 parent b920d8b
commit 3ef90e0
Show file tree

Hide file tree

Showing 7 changed files with 26 additions and 9 deletions.
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -65,6 +65,8 @@ Other Changes
 
 * SOLR-16911: Establish /solr as the only host context supported by Solr, removing legacy ability to change this.  Solr paths will only be either /solr or /api now.  (Eric Pugh)
 
+* SOLR-16995: Add a ReplicaCount class to keep track of replicas per type (Vincent Primault)
+
 ==================  9.5.0 ==================
 New Features
 ---------------------

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
@@ -335,7 +335,7 @@ public static List<ReplicaPosition> getNodesForNewReplicas(
       log.debug(
           "getNodesForNewReplicas() shard={}, {} , createNodeSet={}",
           shard,
-          numReplicas.toDebugString(),
+          numReplicas,
           createNodeSet);
     }
     List<String> createNodeList;

diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
@@ -243,7 +243,7 @@ public boolean split(ClusterState clusterState, ZkNodeProps message, NamedList<O
             "aborting split - inconsistent replica types in collection "
                 + collectionName
                 + ": "
-                + numReplicas.toDebugString()
+                + numReplicas
                 + ", shard leader type is "
                 + leaderReplicaType);
       }

diff --git a/solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java b/solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java
@@ -33,6 +33,21 @@ public class PlacementRequestImpl implements PlacementRequest {
   private final Set<Node> targetNodes;
   private final ReplicaCount numReplicas;
 
+  @Deprecated
+  public PlacementRequestImpl(
+      SolrCollection solrCollection,
+      Set<String> shardNames,
+      Set<Node> targetNodes,
+      int countNrtReplicas,
+      int countTlogReplicas,
+      int countPullReplicas) {
+    this(
+        solrCollection,
+        shardNames,
+        targetNodes,
+        new ReplicaCount(countNrtReplicas, countTlogReplicas, countPullReplicas));
+  }
+
   public PlacementRequestImpl(
       SolrCollection solrCollection,
       Set<String> shardNames,

diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
@@ -519,7 +519,7 @@ protected Create(String collection, String config, String shards, int numNrtRepl
           ImplicitDocRouter.NAME,
           null,
           checkNotNull("shards", shards),
-          new ReplicaCount(numNrtReplicas, 0, 0));
+          ReplicaCount.of(Replica.Type.NRT, numNrtReplicas));
     }
 
     private Create(

diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java
@@ -19,13 +19,12 @@
 import java.util.Arrays;
 import java.util.EnumMap;
 import java.util.Locale;
-import java.util.Map;
 import java.util.stream.Collectors;
 import org.apache.solr.common.SolrException;
 
 /** Tracks the number of replicas per replica type. This class is mutable. */
 public final class ReplicaCount {
-  private final Map<Replica.Type, Integer> countByType;
+  private final EnumMap<Replica.Type, Integer> countByType;
 
   public ReplicaCount(Integer nrtReplicas, Integer tlogReplicas, Integer pullReplicas) {
     this.countByType = new EnumMap<>(Replica.Type.class);
@@ -115,7 +114,7 @@ public Replica.Type getLeaderType() {
     throw new SolrException(
         SolrException.ErrorCode.BAD_REQUEST,
         "Unexpected number of replicas ("
-            + toDebugString()
+            + this
             + "), there must be at least one leader-eligible replica");
   }
 
@@ -125,7 +124,8 @@ public void validate() {
   }
 
   /** Returns a representation of this class which can be used for debugging. */
-  public String toDebugString() {
+  @Override
+  public String toString() {
     return Arrays.stream(Replica.Type.values())
         .map(t -> t.name().toLowerCase(Locale.ROOT) + "=" + get(t))
         .collect(Collectors.joining(", "));

diff --git a/solr/solrj/src/test/org/apache/solr/common/cloud/ReplicaCountTest.java b/solr/solrj/src/test/org/apache/solr/common/cloud/ReplicaCountTest.java
@@ -108,12 +108,12 @@ public void testTotal() {
   }
 
   @Test
-  public void testToDebugString() {
+  public void testToString() {
     ReplicaCount replicaCount = ReplicaCount.empty();
     replicaCount.put(Replica.Type.NRT, 1);
     replicaCount.put(Replica.Type.TLOG, 2);
 
-    assertEquals("nrt=1, tlog=2, pull=0", replicaCount.toDebugString());
+    assertEquals("nrt=1, tlog=2, pull=0", replicaCount.toString());
   }
 
   @Test