SOLR-17153: CloudSolrClient should not throw "Collection not found" w…

…ith an out-dated ClusterState (#2363) ZkClientClusterStateProvider needed to double-check a collection truly doesn't exist. HttpClusterStateProvider is already correct. HttpSolrCall on the server side was hardened similarly. This could happen for a highly burdened cluster / zookeeper.
apache · Apr 3, 2024 · 5c399dd · 5c399dd
1 parent 2eb9b21
commit 5c399dd
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 56 deletions.
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -135,6 +135,8 @@ Optimizations
 
 Bug Fixes
 ---------------------
+* SOLR-17153: CloudSolrClient could fail a request immediately following a collection creation.  Required double-checking the collection doesn’t exist. (Aparna Suresh via David Smiley)
+
 * SOLR-17152: Better alignment of Admin UI graph (janhoy)
 
 * SOLR-17148: Fixing Config API overlay property enabling or disabling the cache (Sanjay Dutt, hossman, Eric Pugh)

diff --git a/solr/core/src/java/org/apache/solr/api/V2HttpCall.java b/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
@@ -35,14 +35,12 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
-import java.util.function.Supplier;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import net.jcip.annotations.ThreadSafe;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.util.JsonSchemaValidator;
 import org.apache.solr.common.util.PathTrie;
@@ -140,8 +138,20 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
       if (pathSegments.size() > 1 && ("c".equals(prefix) || "collections".equals(prefix))) {
         origCorename = pathSegments.get(1);
 
-        DocCollection collection =
-            resolveDocCollection(queryParams.get(COLLECTION_PROP, origCorename));
+        String collectionStr = queryParams.get(COLLECTION_PROP, origCorename);
+        collectionsList =
+            resolveCollectionListOrAlias(collectionStr); // &collection= takes precedence
+        if (collectionsList.size() > 1) {
+          throw new SolrException(
+              SolrException.ErrorCode.BAD_REQUEST,
+              "Request must be sent to a single collection "
+                  + "or an alias that points to a single collection,"
+                  + " but '"
+                  + collectionStr
+                  + "' resolves to "
+                  + this.collectionsList);
+        }
+        DocCollection collection = resolveDocCollection(collectionsList);
         if (collection == null) {
           if (!path.endsWith(CommonParams.INTROSPECT)) {
             throw new SolrException(
@@ -218,54 +228,6 @@ protected void parseRequest() throws Exception {
     if (solrReq == null) solrReq = parser.parse(core, path, req);
   }
 
-  /**
-   * Lookup the collection from the collection string (maybe comma delimited). Also sets {@link
-   * #collectionsList} by side-effect. if {@code secondTry} is false then we'll potentially
-   * recursively try this all one more time while ensuring the alias and collection info is sync'ed
-   * from ZK.
-   */
-  protected DocCollection resolveDocCollection(String collectionStr) {
-    if (!cores.isZooKeeperAware()) {
-      throw new SolrException(
-          SolrException.ErrorCode.BAD_REQUEST, "Solr not running in cloud mode ");
-    }
-    ZkStateReader zkStateReader = cores.getZkController().getZkStateReader();
-
-    Supplier<DocCollection> logic =
-        () -> {
-          this.collectionsList = resolveCollectionListOrAlias(collectionStr); // side-effect
-          if (collectionsList.size() > 1) {
-            throw new SolrException(
-                SolrException.ErrorCode.BAD_REQUEST,
-                "Request must be sent to a single collection "
-                    + "or an alias that points to a single collection,"
-                    + " but '"
-                    + collectionStr
-                    + "' resolves to "
-                    + this.collectionsList);
-          }
-          String collectionName = collectionsList.get(0); // first
-          // TODO an option to choose another collection in the list if can't find a local replica
-          // of the first?
-
-          return zkStateReader.getClusterState().getCollectionOrNull(collectionName);
-        };
-
-    DocCollection docCollection = logic.get();
-    if (docCollection != null) {
-      return docCollection;
-    }
-    // ensure our view is up to date before trying again
-    try {
-      zkStateReader.aliasesManager.update();
-      zkStateReader.forceUpdateCollection(collectionsList.get(0));
-    } catch (Exception e) {
-      log.error("Error trying to update state while resolving collection.", e);
-      // don't propagate exception on purpose
-    }
-    return logic.get();
-  }
-
   public static Api getApiInfo(
       PluginBag<SolrRequestHandler> requestHandlers,
       String path,

diff --git a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
@@ -53,6 +53,7 @@
 import java.util.Random;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import net.jcip.annotations.ThreadSafe;
@@ -281,6 +282,8 @@ protected void init() throws Exception {
               queryParams.get(COLLECTION_PROP, def)); // &collection= takes precedence
 
       if (core == null) {
+        // force update collection only if local clusterstate is outdated
+        resolveDocCollection(collectionsList);
         // lookup core from collection, or route away if need to
         // route to 1st
         String collectionName = collectionsList.isEmpty() ? null : collectionsList.get(0);
@@ -345,6 +348,37 @@ protected void init() throws Exception {
     action = PASSTHROUGH;
   }
 
+  /**
+   * Lookup the collection from the collection string (maybe comma delimited). Also sets {@link
+   * #collectionsList} by side-effect. if {@code secondTry} is false then we'll potentially
+   * recursively try this all one more time while ensuring the alias and collection info is sync'ed
+   * from ZK.
+   */
+  protected DocCollection resolveDocCollection(List<String> collectionsList) {
+    if (!cores.isZooKeeperAware()) {
+      throw new SolrException(
+          SolrException.ErrorCode.BAD_REQUEST, "Solr not running in cloud mode ");
+    }
+    ZkStateReader zkStateReader = cores.getZkController().getZkStateReader();
+    String collectionName = collectionsList.get(0);
+    Supplier<DocCollection> logic =
+        () -> zkStateReader.getClusterState().getCollectionOrNull(collectionName);
+
+    DocCollection docCollection = logic.get();
+    if (docCollection != null) {
+      return docCollection;
+    }
+    // ensure our view is up to date before trying again
+    try {
+      zkStateReader.aliasesManager.update();
+      zkStateReader.forceUpdateCollection(collectionName);
+    } catch (Exception e) {
+      log.error("Error trying to update state while resolving collection.", e);
+      // don't propagate exception on purpose
+    }
+    return logic.get();
+  }
+
   protected void autoCreateSystemColl(String corename) throws Exception {
     if (core == null
         && SYSTEM_COLL.equals(corename)

diff --git a/...rj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java b/...rj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java
@@ -144,11 +144,22 @@ public static ClusterState createFromJsonSupportingLegacyConfigName(
   @Override
   public ClusterState.CollectionRef getState(String collection) {
     ClusterState clusterState = getZkStateReader().getClusterState();
-    if (clusterState != null) {
-      return clusterState.getCollectionRef(collection);
-    } else {
+    if (clusterState == null) {
       return null;
     }
+
+    ClusterState.CollectionRef collectionRef = clusterState.getCollectionRef(collection);
+    if (collectionRef == null) {
+      // force update collection
+      try {
+        getZkStateReader().forceUpdateCollection(collection);
+        return getZkStateReader().getClusterState().getCollectionRef(collection);
+      } catch (KeeperException | InterruptedException e) {
+        return null;
+      }
+    } else {
+      return collectionRef;
+    }
   }
 
   @Override

diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java
@@ -53,7 +53,7 @@ static ClusterStateProvider newZkClusterStateProvider(
   /**
    * Obtain the state of the collection (cluster status).
    *
-   * @return the collection state, or null is collection doesn't exist
+   * @return the collection state, or null only if collection doesn't exist
    */
   ClusterState.CollectionRef getState(String collection);