SOLR-14985: CloudSolrClient with Solr URLs wasn't caching state (#2571)

Solrj CloudSolrClient with Solr URLs had serious performance regressions (since the beginning?) in which its collection state cache was not being used, resulting in many extra requests to Solr for cluster information. (Aparna Suresh, shalin, David Smiley)


Co-authored-by: Shalin Shekhar Mangar <[email protected]>

Author: aparnasuresh85

solr/CHANGES.txt

@@ -111,7 +111,9 @@ Improvements
 
 Optimizations
 ---------------------
-(No changes)
+* SOLR-14985: Solrj CloudSolrClient with Solr URLs had serious performance regressions (since the
+  beginning?) in which its collection state cache was not being used, resulting in many extra
+  requests to Solr for cluster information. (Aparna Suresh, shalin, David Smiley)
 
 Bug Fixes
 ---------------------

solr/solrj/src/java/org/apache/solr/client/solrj/cloud/DelegatingClusterStateProvider.java

@@ -78,6 +78,15 @@ public String resolveSimpleAlias(String alias) throws IllegalArgumentException {
     }
   }
 
+  @Override
+  public Object getClusterProperty(String propertyName) {
+    if (delegate != null) {
+      return delegate.getClusterProperty(propertyName);
+    } else {
+      return null;
+    }
+  }
+
   @Override
   public ClusterState getClusterState() {
     if (delegate != null) {

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

@@ -362,8 +362,9 @@ private NamedList<Object> directUpdate(AbstractUpdateRequest request, String col
 
     // Check to see if the collection is an alias. Updates to multi-collection aliases are ok as
     // long as they are routed aliases
-    List<String> aliasedCollections = getClusterStateProvider().resolveAlias(collection);
-    if (getClusterStateProvider().isRoutedAlias(collection) || aliasedCollections.size() == 1) {
+    List<String> aliasedCollections =
+        new ArrayList<>(resolveAliases(Collections.singletonList(collection)));
+    if (aliasedCollections.size() == 1 || getClusterStateProvider().isRoutedAlias(collection)) {
       collection = aliasedCollections.get(0); // pick 1st (consistent with HttpSolrCall behavior)
     } else {
       throw new SolrException(
@@ -1149,7 +1150,7 @@ private Set<String> resolveAliases(List<String> inputCollections) {
     }
     LinkedHashSet<String> uniqueNames = new LinkedHashSet<>(); // consistent ordering
     for (String collectionName : inputCollections) {
-      if (getClusterStateProvider().getState(collectionName) == null) {
+      if (getDocCollection(collectionName, -1) == null) {
         // perhaps it's an alias
         uniqueNames.addAll(getClusterStateProvider().resolveAlias(collectionName));
       } else {
@@ -1208,15 +1209,6 @@ protected DocCollection getDocCollection(String collection, Integer expectedVers
       if (expectedVersion <= col.getZNodeVersion() && !cacheEntry.shouldRetry()) return col;
     }
 
-    ClusterState.CollectionRef ref = getCollectionRef(collection);
-    if (ref == null) {
-      // no such collection exists
-      return null;
-    }
-    if (!ref.isLazilyLoaded()) {
-      // it is readily available just return it
-      return ref.get();
-    }
     Object[] locks = this.locks;
     int lockId =
         Math.abs(Hash.murmurhash3_x86_32(collection, 0, collection.length(), 0) % locks.length);
@@ -1228,6 +1220,11 @@ protected DocCollection getDocCollection(String collection, Integer expectedVers
       if (col != null) {
         if (expectedVersion <= col.getZNodeVersion() && !cacheEntry.shouldRetry()) return col;
       }
+      ClusterState.CollectionRef ref = getCollectionRef(collection);
+      if (ref == null) {
+        // no such collection exists
+        return null;
+      }
       // We are going to fetch a new version
       // we MUST try to get a new version
       DocCollection fetchedCol = ref.get(); // this is a call to ZK

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

@@ -108,15 +108,13 @@ default DocCollection getCollection(String name) throws IOException {
   /** Obtain a cluster property, or the default value if it doesn't exist. */
   default <T> T getClusterProperty(String key, T defaultValue) {
     @SuppressWarnings({"unchecked"})
-    T value = (T) getClusterProperties().get(key);
+    T value = (T) getClusterProperty(key);
     if (value == null) return defaultValue;
     return value;
   }
 
   /** Obtain a cluster property, or null if it doesn't exist. */
-  default Object getClusterProperty(String propertyName) {
-    return getClusterProperties().get(propertyName);
-  }
+  Object getClusterProperty(String propertyName);
 
   /** Get the collection-specific policy */
   String getPolicyNameByCollection(String coll);

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

@@ -35,6 +35,7 @@
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.regex.Pattern;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.lucene.tests.util.TestUtil;
 import org.apache.solr.client.solrj.SolrClient;
@@ -70,6 +71,9 @@
 import org.apache.solr.handler.admin.CollectionsHandler;
 import org.apache.solr.handler.admin.ConfigSetsHandler;
 import org.apache.solr.handler.admin.CoreAdminHandler;
+import org.apache.solr.servlet.HttpSolrCall;
+import org.apache.solr.util.LogLevel;
+import org.apache.solr.util.LogListener;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -89,6 +93,15 @@ public class CloudHttp2SolrClientTest extends SolrCloudTestCase {
   private static CloudSolrClient httpBasedCloudSolrClient = null;
   private static CloudSolrClient zkBasedCloudSolrClient = null;
 
+  private static final Pattern PATTERN_WITH_COLLECTION =
+      Pattern.compile(
+          "path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS"
+              + "[^}]*collection=[^&}]+[^}]*\\}");
+  private static final Pattern PATTERN_WITHOUT_COLLECTION =
+      Pattern.compile(
+          "path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS"
+              + "(?![^}]*collection=)[^}]*\\}");
+
   @BeforeClass
   public static void setupCluster() throws Exception {
     System.setProperty("metricsEnabled", "true");
@@ -114,6 +127,7 @@ public static void setupCluster() throws Exception {
 
   @AfterClass
   public static void tearDownAfterClass() throws Exception {
+
     if (httpBasedCloudSolrClient != null) {
       try {
         httpBasedCloudSolrClient.close();
@@ -246,6 +260,57 @@ public void testAliasHandling() throws Exception {
         2, client.query(null, paramsWithMixedCollectionAndAlias).getResults().getNumFound());
   }
 
+  @Test
+  @LogLevel("org.apache.solr.servlet.HttpSolrCall=DEBUG")
+  public void testHttpCspPerf() throws Exception {
+
+    String collectionName = "HTTPCSPTEST";
+    CollectionAdminRequest.createCollection(collectionName, "conf", 2, 1)
+        .process(cluster.getSolrClient());
+    cluster.waitForActiveCollection(collectionName, 2, 2);
+
+    try (LogListener entireClusterStateLogs =
+            LogListener.info(HttpSolrCall.class).regex(PATTERN_WITHOUT_COLLECTION);
+        LogListener collectionClusterStateLogs =
+            LogListener.info(HttpSolrCall.class).regex(PATTERN_WITH_COLLECTION);
+        LogListener adminRequestLogs = LogListener.info(HttpSolrCall.class).substring("[admin]");
+        CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient(); ) {
+      SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my doc");
+      solrClient.add(collectionName, doc);
+      solrClient.commit(collectionName);
+      for (int i = 0; i < 3; i++) {
+        assertEquals(
+            1, solrClient.query(collectionName, params("q", "*:*")).getResults().getNumFound());
+      }
+
+      // 1 call to fetch entire cluster state via BaseHttpCSP.fetchLiveNodes()
+      // 1 call to fetch CLUSTERSTATUS for collection via getDocCollection() (first collection
+      // lookup)
+      assertLogCount(adminRequestLogs, 2);
+      // 1 call to fetch CLUSTERSTATUS for collection via getDocCollection() (first collection
+      // lookup)
+      assertLogCount(collectionClusterStateLogs, 1);
+      // 1 call to fetch entire cluster state from HttpCSP.fetchLiveNodes()
+      assertLogCount(entireClusterStateLogs, 1);
+    }
+  }
+
+  private CloudSolrClient createHttpCSPBasedCloudSolrClient() {
+    final List<String> solrUrls = new ArrayList<>();
+    solrUrls.add(cluster.getJettySolrRunner(0).getBaseUrl().toString());
+    return new CloudHttp2SolrClient.Builder(solrUrls).build();
+  }
+
+  private void assertLogCount(LogListener logListener, int expectedCount) {
+    int logCount = logListener.getCount();
+    assertEquals(expectedCount, logCount);
+    if (logCount > 0) {
+      for (int i = 0; i < logCount; i++) {
+        logListener.pollMessage();
+      }
+    }
+  }
+
   @Test
   public void testRouting() throws Exception {
     CollectionAdminRequest.createCollection("routing_collection", "conf", 2, 1)