SOLR-17535: Deprecate ClusterState.forEachCollection (#2854)

Use collectionStream() instead. Redirect callers. A simple refactoring.
apache · Nov 12, 2024 · c694258 · c694258
1 parent 981f678
commit c694258
Show file tree

Hide file tree

Showing 6 changed files with 62 additions and 49 deletions.
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -227,7 +227,8 @@ led to the suppression of exceptions. (Andrey Bozhko)
 
 * SOLR-17534: Introduce ClusterState.getCollectionNames, a convenience method (David Smiley)
 
-* SOLR-17535: Introduce ClusterState.collectionStream to replace getCollectionStates and getCollectionsMap (David Smiley)
+* SOLR-17535: Introduce ClusterState.collectionStream to replace getCollectionStates, getCollectionsMap,
+  and forEachCollection, which are now deprecated. (David Smiley)
 
 * SOLR-17545: Upgrade to Gradle 8.10 (Houston Putman)
 

diff --git a/solr/core/src/java/org/apache/solr/cluster/events/impl/CollectionsRepairEventListener.java b/solr/core/src/java/org/apache/solr/cluster/events/impl/CollectionsRepairEventListener.java
@@ -40,7 +40,6 @@
 import org.apache.solr.cluster.events.ClusterEvent;
 import org.apache.solr.cluster.events.ClusterEventListener;
 import org.apache.solr.cluster.events.NodesDownEvent;
-import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.ReplicaCount;
 import org.apache.solr.common.cloud.ReplicaPosition;
 import org.apache.solr.common.util.SolrNamedThreadFactory;
@@ -168,38 +167,44 @@ private void runRepair() {
     // collection / positions
     Map<String, List<ReplicaPosition>> newPositions = new HashMap<>();
     try {
-      ClusterState clusterState = solrCloudManager.getClusterState();
-      clusterState.forEachCollection(
-          coll -> {
-            // shard / number of replicas per type
-            Map<String, ReplicaCount> lostReplicas = new HashMap<>();
-            coll.forEachReplica(
-                (shard, replica) -> {
-                  if (reallyLostNodes.contains(replica.getNodeName())) {
-                    lostReplicas
-                        .computeIfAbsent(shard, s -> ReplicaCount.empty())
-                        .increment(replica.type);
-                  }
-                });
-            Assign.AssignStrategy assignStrategy = Assign.createAssignStrategy(cc);
-            lostReplicas.forEach(
-                (shard, types) -> {
-                  Assign.AssignRequest assignRequest =
-                      new Assign.AssignRequestBuilder()
-                          .forCollection(coll.getName())
-                          .forShard(Collections.singletonList(shard))
-                          .assignReplicas(types)
-                          .build();
-                  try {
-                    List<ReplicaPosition> positions =
-                        assignStrategy.assign(solrCloudManager, assignRequest);
-                    newPositions.put(coll.getName(), positions);
-                  } catch (Exception e) {
-                    log.warn(
-                        "Exception computing positions for {}/{}: {}", coll.getName(), shard, e);
-                  }
-                });
-          });
+      // shard / number of replicas per type
+      solrCloudManager
+          .getClusterState()
+          .collectionStream()
+          .forEach(
+              coll -> {
+                // shard / number of replicas per type
+                Map<String, ReplicaCount> lostReplicas = new HashMap<>();
+                coll.forEachReplica(
+                    (shard, replica) -> {
+                      if (reallyLostNodes.contains(replica.getNodeName())) {
+                        lostReplicas
+                            .computeIfAbsent(shard, s -> ReplicaCount.empty())
+                            .increment(replica.type);
+                      }
+                    });
+                Assign.AssignStrategy assignStrategy = Assign.createAssignStrategy(cc);
+                lostReplicas.forEach(
+                    (shard, types) -> {
+                      Assign.AssignRequest assignRequest =
+                          new Assign.AssignRequestBuilder()
+                              .forCollection(coll.getName())
+                              .forShard(Collections.singletonList(shard))
+                              .assignReplicas(types)
+                              .build();
+                      try {
+                        List<ReplicaPosition> positions =
+                            assignStrategy.assign(solrCloudManager, assignRequest);
+                        newPositions.put(coll.getName(), positions);
+                      } catch (Exception e) {
+                        log.warn(
+                            "Exception computing positions for {}/{}: {}",
+                            coll.getName(),
+                            shard,
+                            e);
+                      }
+                    });
+              });
     } catch (IOException e) {
       log.warn("Exception getting cluster state", e);
       return;

diff --git a/solr/core/src/test/org/apache/solr/cloud/ReindexCollectionTest.java b/solr/core/src/test/org/apache/solr/cloud/ReindexCollectionTest.java
@@ -367,7 +367,8 @@ public void testFailure() throws Exception {
     // verify that the target and checkpoint collections don't exist
     cloudManager
         .getClusterState()
-        .forEachCollection(
+        .collectionStream()
+        .forEach(
             coll -> {
               assertFalse(
                   coll.getName() + " still exists",

diff --git a/...lrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java b/...lrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java
@@ -78,18 +78,21 @@ protected void readReplicaDetails() throws IOException {
     if (clusterState == null) { // zkStateReader still initializing
       return;
     }
-    clusterState.forEachCollection(
-        coll ->
-            coll.forEachReplica(
-                (shard, replica) -> {
-                  Map<String, Map<String, List<Replica>>> nodeData =
-                      nodeVsCollectionVsShardVsReplicaInfo.computeIfAbsent(
-                          replica.getNodeName(), k -> new HashMap<>());
-                  Map<String, List<Replica>> collData =
-                      nodeData.computeIfAbsent(coll.getName(), k -> new HashMap<>());
-                  List<Replica> replicas = collData.computeIfAbsent(shard, k -> new ArrayList<>());
-                  replicas.add((Replica) replica.clone());
-                }));
+    clusterState
+        .collectionStream()
+        .forEach(
+            coll ->
+                coll.forEachReplica(
+                    (shard, replica) -> {
+                      Map<String, Map<String, List<Replica>>> nodeData =
+                          nodeVsCollectionVsShardVsReplicaInfo.computeIfAbsent(
+                              replica.getNodeName(), k -> new HashMap<>());
+                      Map<String, List<Replica>> collData =
+                          nodeData.computeIfAbsent(coll.getName(), k -> new HashMap<>());
+                      List<Replica> replicas =
+                          collData.computeIfAbsent(shard, k -> new ArrayList<>());
+                      replicas.add((Replica) replica.clone());
+                    }));
   }
 
   @Override

diff --git a/solr/solrj-zookeeper/src/java/org/apache/solr/common/LazySolrCluster.java b/solr/solrj-zookeeper/src/java/org/apache/solr/common/LazySolrCluster.java
@@ -170,8 +170,8 @@ public SolrCollection get(String key) {
       public void forEachEntry(BiConsumer<String, ? super SolrCollection> fun) {
         zkStateReader
             .getClusterState()
-            .forEachCollection(
-                coll -> fun.accept(coll.getName(), _collection(coll.getName(), coll)));
+            .collectionStream()
+            .forEach(coll -> fun.accept(coll.getName(), _collection(coll.getName(), coll)));
       }
 
       @Override

diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
@@ -425,7 +425,10 @@ public Stream<DocCollection> collectionStream() {
   /**
    * Calls {@code consumer} with a resolved {@link DocCollection}s for all collections. Use this
    * sparingly in case there are many collections.
+   *
+   * @deprecated see {@link #collectionStream()}
    */
+  @Deprecated
   public void forEachCollection(Consumer<DocCollection> consumer) {
     collectionStream().forEach(consumer);
   }