SOLR-17535: Migrate ClusterState.getCollectionsMap callers to Stream #2846

dsmiley · 2024-11-05T18:22:04Z

https://issues.apache.org/jira/browse/SOLR-17535

This PR builds on top of the previous PR #2834 , you can ignore changes here for that (the first set of commits on this PR) as that's reviewed separately.

Deprecated the latter.

…tatesNoMap

Overhaul ClusterStateUtil; optimize for when one collection is specified by calling ZkStateReader.waitForState.

murblanc

Left a few comments, looks good!

murblanc · 2024-11-05T22:01:21Z

solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java

+                .getClusterState()
+                .collectionStream()
+                .map(DocCollection::getConfigName)
+                .anyMatch(cf -> cf.equals(configSetName));


Maybe do equals() in the other direction to prevent NPE if cf is null?

murblanc · 2024-11-05T22:06:53Z

solr/core/src/java/org/apache/solr/cloud/overseer/NodeMutator.java

+        .filter(entry -> !entry.isPerReplicaState())
+        .map(
+            docCollection ->
+                computeCollectionUpdate(nodeName, docCollection.getName(), docCollection, zkClient))


Maybe change the signature of computeCollectionUpdate() to get the name from the passed in DocCollection and save one argument here?

murblanc · 2024-11-05T22:14:09Z

solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java

-    if (collection == null) {
-      collectionsMap = clusterState.getCollectionsMap();
+    Stream<DocCollection> collectionStream;
+    if (collection == null) { // uh-oh; hopefully not a lot


I assume you mean that cluster status is going to return a lot of data and we don't like it, but the comment doesn't really help to understand that (and if that's the actual need, it's not an issue, just a price to pay...)

Unrelated, but this method that was already hard to read and I find the changes below make it even harder to read/maintain.
Maybe it's just me though.

I made some slight changes that maybe helps readability more. I didn't find it straight-forward previously.

murblanc · 2024-11-05T22:26:22Z

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

+          .collectionStream()
+          .map(DocCollection::getActiveSlicesArr)
+          .filter(Objects::nonNull)
+          .forEach(them -> Collections.addAll(slices, them));


maybe use another variable name than them that doesn't convey much meaning?

slices would be logical but that's already in-scope; it's an "out" argument. Despite the scope creep, I refactored further to eliminate the argument. I think it's overall clearer.

murblanc · 2024-11-05T22:29:48Z

solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java

@@ -1067,6 +1068,10 @@ public void testDeleteAliasedCollection() throws Exception {
        });
  }

+  private static String toString(ClusterState state) {


I'd name this collectionNames or something with more meaning than toString

Sure. BTW it's purely used for printing on an assert failure.

murblanc · 2024-11-05T22:34:08Z

...odules/hdfs/src/test/org/apache/solr/hdfs/cloud/SharedFileSystemAutoReplicaFailoverTest.java

-    }
-
-    return success;
+    return ClusterStateUtil.waitFor(


Nice cleanup!

murblanc · 2024-11-05T22:48:34Z

solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java

   */
+  @Deprecated


Are there still uses of getCollectionsMap() in the code? If not, I suggest to remove this method rather than deprecate it. This is not a public API, right?
If references remain, why not remove them?
Same comment for getCollectionStates() below.

They are public and are surely called by our users (and our users internally :sad:). On a subsequent PR that will only occur on main (10x), I will remove them. I will also remove them in our fork.

murblanc · 2024-11-05T23:05:36Z

solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java

+
+  /**
+   * Calls {@code consumer} with a resolved {@link DocCollection}s for all collections. Use this
+   * sparingly in case there are many collections.
   */
  public void forEachCollection(Consumer<DocCollection> consumer) {


I wonder if we should keep this method now that we have collectionStream().

I agree 100% to remove this as well; I forgot to raise this point and I held myself back from scope creep in doing so in this PR. I'll do a fast followup for that little change; same JIRA issue.

murblanc · 2024-11-05T23:18:37Z

solr/test-framework/src/java/org/apache/solr/common/cloud/ClusterStateUtil.java

-          throw new SolrException(ErrorCode.SERVER_ERROR, "Interrupted");
+    final long timeoutAtNs =
+        System.nanoTime() + TimeUnit.NANOSECONDS.convert(timeoutMs, TimeUnit.MILLISECONDS);
+    while (System.nanoTime() < timeoutAtNs) {


Maybe change into a do...while so that:

It runs at least once even if timeoutMs is 0 (or non 0 + an improbable GC at the wrong moment)

It doesn't sleep then fails directly without checking again if predicate matches when the timeout is exceeded

dsmiley · 2024-11-06T02:56:06Z

solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java

@@ -168,7 +168,7 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
      }

      // delete related config set iff: it is auto generated AND not related to any other collection
-      String configSetName = coll.getConfigName();
+      String configSetName = coll == null ? null : coll.getConfigName();


IntelliJ helpfully pointed out an issue nearby this PR with a trivial fix. Basically, it might not have been possible to delete a non-existent collection but now it might be. (couldn't find a test for this)

dsmiley · 2024-11-06T02:58:14Z

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

-    List<Slice> activeSlices = new ArrayList<>();
+    List<Slice> activeSlices;


I really liked this little improvement to this method clarify that we produce the activeSlices once; it's not built up / amended to a growing list (which wasn't super obvious)

dsmiley · 2024-11-06T02:59:45Z

solr/test-framework/src/java/org/apache/solr/common/cloud/ClusterStateUtil.java

+   * has an alternative implementation if {@code collection} is null, in which the predicate must
+   * match *all* collections. Returns whether the predicate matches or not in the allotted time;
+   * does *NOT* throw {@link TimeoutException}.
+   */
  public static boolean waitFor(


I aligned the parameters to be closer to that of ZkStateReader.waitForState

…allers # Conflicts: # solr/CHANGES.txt # solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java

…allers

…2846) Deprecated getCollectionsMap. A refactoring, albeit some additional points: * DeleteCollection: avoid NPE for non-existent collection (trivial change) * ClusterStateUtil (test-framework): optimize waiting to use ZkStateReader.waitForState for single-collection (cherry picked from commit 24fe191)

dsmiley added 6 commits November 1, 2024 16:35

ClusterState.collectionStream() in lieu of getCollectionStates()

4f2fbca

Deprecated the latter.

better javadocs

3dca7f3

Merge remote-tracking branch 'refs/remotes/apache/main' into clusterS…

5ab9215

…tatesNoMap

javadoc links to DocCollection

490aede

CHANGES.txt

d695f6d

SOLR-17535: Migrate ClusterState.getCollectionsMap callers to Stream

dfbc3c1

Overhaul ClusterStateUtil; optimize for when one collection is specified by calling ZkStateReader.waitForState.

dsmiley requested a review from murblanc November 5, 2024 18:22

github-actions bot added test-framework jetty-server client:solrj module:hdfs prometheus-exporter tests cat:cloud cat:api labels Nov 5, 2024

murblanc approved these changes Nov 5, 2024

View reviewed changes

dsmiley added 4 commits November 5, 2024 21:50

DeleteCollectionCmd: avoid NPE for non-existent collection

8da453e

HttpSolrCall: clarity to getSlicesForAllCollections

e56a757

ClusterStateUtil.waitFor improvements

d8f38cf

misc feedback

9797d41

dsmiley commented Nov 6, 2024

View reviewed changes

dsmiley added 4 commits November 7, 2024 22:50

Merge branch 'refs/heads/main' into clusterStatesNoGetCollectionsMapC…

586fd9a

…allers # Conflicts: # solr/CHANGES.txt # solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java

Merge branch 'refs/heads/main' into clusterStatesNoGetCollectionsMapC…

690954d

…allers

slices is unmodifiable; need to clone it

cfeb923

Fix printClusterStateInfo details

177dd59

dsmiley merged commit 24fe191 into apache:main Nov 13, 2024
5 checks passed

dsmiley deleted the clusterStatesNoGetCollectionsMapCallers branch November 13, 2024 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17535: Migrate ClusterState.getCollectionsMap callers to Stream #2846

SOLR-17535: Migrate ClusterState.getCollectionsMap callers to Stream #2846

dsmiley commented Nov 5, 2024

murblanc left a comment

murblanc Nov 5, 2024

murblanc Nov 5, 2024

murblanc Nov 5, 2024

murblanc Nov 5, 2024

dsmiley Nov 6, 2024

murblanc Nov 5, 2024

dsmiley Nov 6, 2024

murblanc Nov 5, 2024

dsmiley Nov 6, 2024

murblanc Nov 5, 2024

murblanc Nov 5, 2024

dsmiley Nov 6, 2024

murblanc Nov 5, 2024

dsmiley Nov 6, 2024

murblanc Nov 5, 2024

dsmiley Nov 6, 2024

dsmiley Nov 6, 2024

dsmiley Nov 6, 2024

		List<Slice> activeSlices = new ArrayList<>();
		List<Slice> activeSlices;

SOLR-17535: Migrate ClusterState.getCollectionsMap callers to Stream #2846

SOLR-17535: Migrate ClusterState.getCollectionsMap callers to Stream #2846

Conversation

dsmiley commented Nov 5, 2024

murblanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment