From a24b4866caf681fcf4ca8491c4ed1d94995317b5 Mon Sep 17 00:00:00 2001 From: aparnasuresh85 Date: Thu, 21 Mar 2024 09:29:24 -0400 Subject: [PATCH 1/5] CloudSolrClient should not throw "Collection not found" with an outdated cluster state --- .../java/org/apache/solr/api/V2HttpCall.java | 66 ++++--------------- .../org/apache/solr/servlet/HttpSolrCall.java | 38 +++++++++++ .../impl/ZkClientClusterStateProvider.java | 17 ++++- 3 files changed, 66 insertions(+), 55 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/api/V2HttpCall.java b/solr/core/src/java/org/apache/solr/api/V2HttpCall.java index 4f17f3ea8000..20cec8f846cb 100644 --- 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 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 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 index e93c412676e9..050ac6a81cf8 100644 --- 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,41 @@ 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 collectionsList) { + if (!cores.isZooKeeperAware()) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "Solr not running in cloud mode "); + } + ZkStateReader zkStateReader = cores.getZkController().getZkStateReader(); + Supplier logic = + () -> { + 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(); + } + protected void autoCreateSystemColl(String corename) throws Exception { if (core == null && SYSTEM_COLL.equals(corename) diff --git a/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java b/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java index f9d202f59498..bc56881e86d4 100644 --- a/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java +++ b/solr/solrj-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 From 68333abb38f1f472e7f035163c495ca0b63963c8 Mon Sep 17 00:00:00 2001 From: aparnasuresh85 Date: Thu, 21 Mar 2024 15:28:17 -0400 Subject: [PATCH 2/5] minor refactoring --- .../src/java/org/apache/solr/servlet/HttpSolrCall.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java index 050ac6a81cf8..5fe341dbe51c 100644 --- a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java +++ b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java @@ -360,13 +360,9 @@ protected DocCollection resolveDocCollection(List collectionsList) { SolrException.ErrorCode.BAD_REQUEST, "Solr not running in cloud mode "); } ZkStateReader zkStateReader = cores.getZkController().getZkStateReader(); + String collectionName = collectionsList.get(0); Supplier logic = - () -> { - 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); - }; + () -> zkStateReader.getClusterState().getCollectionOrNull(collectionName); DocCollection docCollection = logic.get(); if (docCollection != null) { @@ -375,7 +371,7 @@ protected DocCollection resolveDocCollection(List collectionsList) { // ensure our view is up to date before trying again try { zkStateReader.aliasesManager.update(); - zkStateReader.forceUpdateCollection(collectionsList.get(0)); + zkStateReader.forceUpdateCollection(collectionName); } catch (Exception e) { log.error("Error trying to update state while resolving collection.", e); // don't propagate exception on purpose From 112e3f43f8793452676c38eb50fa8ae40224d16e Mon Sep 17 00:00:00 2001 From: aparnasuresh85 Date: Thu, 21 Mar 2024 19:13:48 -0400 Subject: [PATCH 3/5] minor refactoring --- .../org/apache/solr/client/solrj/impl/ClusterStateProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index e6b7f2097a44..81bb885c38ba 100644 --- 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); From 465eb97d3df187b0b6239558a827d793f059ce90 Mon Sep 17 00:00:00 2001 From: aparnasuresh85 Date: Tue, 2 Apr 2024 15:22:53 -0400 Subject: [PATCH 4/5] CHANGES.txt --- solr/CHANGES.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index e52b79e87a91..34a4d957220b 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -125,6 +125,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) From e9c2cecae2dfa91c5ce4cd0cbecee430e28bf02e Mon Sep 17 00:00:00 2001 From: aparnasuresh85 Date: Tue, 2 Apr 2024 18:25:44 -0400 Subject: [PATCH 5/5] Commit after tidy --- solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java index 5fe341dbe51c..2e5856f9ad08 100644 --- a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java +++ b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java @@ -282,7 +282,7 @@ 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 + // force update collection only if local clusterstate is outdated resolveDocCollection(collectionsList); // lookup core from collection, or route away if need to // route to 1st