SOLR-17381 SolrJ fix to fetch entire ClusterState if asked #2853

aparnasuresh85 · 2024-11-07T18:21:14Z

Description

Address a test failure in ClusterStateProviderTest.testGetClusterStatus when ClusterStateProvider (CSP) chosen is Http2ClusterStateProvider. More importantly, while ClusterStateProvider.getClusterStatus() is highly discouraged, it should still be functional since there are codepaths using it.

Solution

If a call to get CLUSTERSTATUS intending to get collections data specifies a null value for collection parameter, proceed to fetch entire cluster status since fetching the remaining (live_nodes, cluster properties, roles etc) should not be time consuming anyway.

Tests

Ensure ClusterStateProviderTest.testGetClusterState() passes when chosen CSP is Http2ClusterStateProvider. (Currently the test fails with a NPE)

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

dsmiley

minor: These params.set calls are passing "true" | "false" but there's a nice boolean (i.e. a "sugar" method) for the primitive. Could you change that please? It's a minor matter I know but you wrote this code not long ago.

I understand how this change fixes the bug but it feels like the solution optimizes for the smallest possible adequate fix. I look at the method's caller, fetchClusterState. By its name, it should be fetching the cluster's state, but it's kind of working in two modes -- everything or one collection. Can we have fetchClusterState do only the whole cluster (and pass null to submitClusterStateRequest for the request type to signify everything) and have a different/new fetchCollectionState method that only gets the state of one collection, returning a DocCollection? That will be the common code path, and this recommendation I propose would clarify what little work is pertinent to that code path. If this seems like too much, feel free to push back.

Also, I observe that the clusterProperties param is unused (always given null, I mean). Unsure if this is a recent bug/oversight.

dsmiley

I don't want my feedback to seen as blocking this fix; maybe this should be merged and we move on.

(notwithstanding "tidy" of course)

dsmiley

This is so much better!

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

dsmiley

Excellent

dsmiley · 2024-12-06T21:47:27Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

+      String collectionName = e.getKey();
+      Map<String, Object> collStateMap = (Map<String, Object>) e.getValue();
+      cs =
+          cs.copyWith(


Calling copyWith in a loop of collections is O(N^2) (N=#collections); right? Not a problem introduced in this PR but I just want to confer with you on this problem.

Yes it is indeed O(N^2).

* Use GenericSolrRequest not QueryRequest * Removed unused collection property loading

…ailure

dsmiley · 2024-12-07T22:15:21Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

    List<String> liveNodesList = (List<String>) cluster.get("live_nodes");
    if (liveNodesList != null) {
-      Set<String> liveNodes = new HashSet<>(liveNodesList);
-      this.liveNodes = liveNodes;
+      this.liveNodes = Set.copyOf(liveNodesList);


small optimization

dsmiley · 2024-12-07T22:16:07Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

+    Map<String, DocCollection> collStateByName = new LinkedHashMap<>(collectionsNl.size());
+    for (Entry<String, NamedList<?>> entry : collectionsNl) {
+      final var collStateMap = entry.getValue().asMap(10);
+      final int zNodeVersion = (int) collStateMap.get("znodeVersion");


fixed a critical bug in your last version. We must use the ZK version for the DocCollection, not a bogus -1!

dsmiley · 2024-12-07T22:16:45Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

+    var collectionsNl = (NamedList<NamedList<?>>) cluster.get("collections");
+
+    Map<String, DocCollection> collStateByName = new LinkedHashMap<>(collectionsNl.size());


No longer using ClusterState.copyWith, solving a perf problem

dsmiley · 2024-12-07T22:17:09Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

-    if (clusterProperties != null) {
-      Map<String, Object> properties = (Map<String, Object>) cluster.get("properties");
-      if (properties != null) {
-        clusterProperties.putAll(properties);
-      }


this was dead code; nobody calls this method with clusterProperties anymore

* remove useless singletonMap * liveNodes immutable

Test equivalency.

dsmiley

Should be ready, assuming tests pass. @aparnasuresh85 happy to review with you.

dsmiley · 2024-12-08T17:04:47Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java


-  @SuppressWarnings({"rawtypes", "unchecked"})
-  private DocCollection fillPrs(


I inlined fillPrs into getDocCollectionFromObjects and added a bit more responsibility here so that the callers needn't handle zk version.

dsmiley · 2024-12-08T17:06:04Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

+    params.set("action", "CLUSTERSTATUS");
+
+    params.set("includeAll", false); // will flip flor CLUSTER_STATE
+    switch (requestType) {


switch is more idiomatic

dsmiley · 2024-12-08T22:53:48Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java

@@ -117,4 +149,39 @@ private Instant getCreationTimeFromClusterStatus(String collectionName)
    Map<String, Object> collection = (Map<String, Object>) collections.get(collectionName);
    return Instant.ofEpochMilli((long) collection.get("creationTimeMillis"));
  }
+
+  @Test
+  public void testClusterStateProvider() throws SolrServerException, IOException {


In the process of writing this test, I discovered some keys like "health" in the base ZkNodeProps that prevented equivalency with the ZK one. So I enhanced HttpCSP to remove these keys.

dsmiley · 2024-12-08T22:54:33Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java

Refactored this test to a parameterized test to guarantee we test with both impls.

dsmiley · 2024-12-08T22:56:49Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java

    return new ZkClientClusterStateProvider(cluster.getZkStateReader());
  }

+  private final Supplier<ClusterStateProvider> cspSupplier;
+
+  public ClusterStateProviderTest(String method) throws Exception {


by using a String arg that refers to a method, the printed test execution is clear as to what is being invoked. This uses reflection allows deferred execution of construction of the CSP, which is useful.

dsmiley · 2024-12-08T22:58:30Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java

+      case FETCH_NODE_ROLES -> params.set("roles", true);
+    }
+    params.set("prs", true);
+    var request = new GenericSolrRequest(METHOD.GET, "/admin/collections", params);


formerly was QueryRequest which was wrong even though it apparently worked.

aparnasuresh85 · 2024-12-09T15:53:22Z

Should be ready, assuming tests pass. @aparnasuresh85 happy to review with you.

LGTM - could be merged when all test pass

dsmiley · 2024-12-09T16:21:19Z

The test failure is known flaky
I'll merge.

#2853) When using the HTTP ClusterStateProvider (not ZK), getClusterState() wasn't working correctly; a regression from the first PR for this JIRA issue (not released). Also, * Optimization: fix O(N^2) algorithm to be O(N) for the number of collections when calling getClusterState * liveNodes is now immutable, and probably a non-sorted ordering * removed "health" and some other keys from DocCollection that aren't present when using ZK CSP Minor details: * clearly differentiate internal ClusterState processing from getting one DocCollection * Use GenericSolrRequest not QueryRequest * test the both CSPs better --------- Co-authored-by: David Smiley <dsmiley@salesforce.com> (cherry picked from commit d2045f6)

Allow the possibility to fetch entire cluster state

fe9e043

github-actions bot added the client:solrj label Nov 7, 2024

dsmiley reviewed Nov 7, 2024

View reviewed changes

dsmiley changed the title ~~Allow the possibility to fetch entire cluster state~~ SOLR-17381 fix to fetch entire cluster state if asked Nov 7, 2024

dsmiley changed the title ~~SOLR-17381 fix to fetch entire cluster state if asked~~ SOLR-17381 SolrJ fix to fetch entire ClusterState if asked Nov 7, 2024

dsmiley approved these changes Dec 3, 2024

View reviewed changes

aparnasuresh85 added 2 commits December 6, 2024 12:53

Merge remote-tracking branch 'upstream/main' into address-testfailure

f40fafc

Refactor codepath to fetch entire cluster status vs a single collection

cf1bc04

dsmiley reviewed Dec 6, 2024

View reviewed changes

aparnasuresh85 added 2 commits December 6, 2024 14:11

refactor

fce0b7d

rename variable

182b449

dsmiley approved these changes Dec 6, 2024

View reviewed changes

dsmiley added 3 commits December 7, 2024 16:17

tidy

ca217ce

* Optimization remove O(N^2) on collections

63dc069

* Use GenericSolrRequest not QueryRequest * Removed unused collection property loading

Merge branch 'refs/heads/main' into fork/aparnasuresh85/address-testf…

5a3978e

…ailure

dsmiley reviewed Dec 8, 2024

View reviewed changes

dsmiley added 2 commits December 8, 2024 00:51

* inline fillPRS to getDocCollectionFromObjects

7f8412d

* remove useless singletonMap * liveNodes immutable

Remove name-value pairs that shouldn't be in the final result.

3cf330f

Test equivalency.

github-actions bot added the tests label Dec 8, 2024

dsmiley reviewed Dec 8, 2024

View reviewed changes

dsmiley merged commit d2045f6 into apache:main Dec 9, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17381 SolrJ fix to fetch entire ClusterState if asked #2853

SOLR-17381 SolrJ fix to fetch entire ClusterState if asked #2853

aparnasuresh85 commented Nov 7, 2024

dsmiley left a comment

dsmiley left a comment

dsmiley left a comment

dsmiley left a comment

dsmiley Dec 6, 2024

aparnasuresh85 Dec 6, 2024

dsmiley Dec 7, 2024

dsmiley Dec 7, 2024

dsmiley Dec 7, 2024

dsmiley Dec 7, 2024

dsmiley left a comment

dsmiley Dec 8, 2024

dsmiley Dec 8, 2024

dsmiley Dec 8, 2024

dsmiley Dec 8, 2024

dsmiley Dec 8, 2024

dsmiley Dec 8, 2024

aparnasuresh85 commented Dec 9, 2024

dsmiley commented Dec 9, 2024

		var collectionsNl = (NamedList<NamedList<?>>) cluster.get("collections");

		Map<String, DocCollection> collStateByName = new LinkedHashMap<>(collectionsNl.size());


		@SuppressWarnings({"rawtypes", "unchecked"})
		private DocCollection fillPrs(

SOLR-17381 SolrJ fix to fetch entire ClusterState if asked #2853

SOLR-17381 SolrJ fix to fetch entire ClusterState if asked #2853

Conversation

aparnasuresh85 commented Nov 7, 2024

Description

Solution

Tests

Checklist

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley 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

dsmiley 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

aparnasuresh85 commented Dec 9, 2024

dsmiley commented Dec 9, 2024