[lucene] 01/01: SOLR-14985: Slow indexing and search performance when using HttpClusterStateProvider

[dw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dweiss pushed a commit to branch jira/solr-14985
in repository https://gitbox.apache.org/repos/asf/lucene.git

commit f407de6631c005e14b80eb05bb98948fe0c46511
Author: Shalin Shekhar Mangar <sh...@apache.org>
AuthorDate: Wed Nov 18 19:24:18 2020 +0530

    SOLR-14985: Slow indexing and search performance when using HttpClusterStateProvider
    
    This commit fixes BaseCloudSolrClient to cache collection states returned by HttpClusterStateProvider, reduce the number of calls to getClusterProperty in case of admin requests, replace usage of getClusterStateProvider().getState() with getDocCollection() which caches the collection state. Therefore the number of clusterstatus calls are reduced from 4 for each query/indexing to either one at max and usually 0 (if data is cached already).
---
 solr/CHANGES.txt                                   |  2 +
 .../client/solrj/impl/BaseCloudSolrClient.java     | 13 +--
 .../impl/CountingHttpClusterStateProvider.java     | 72 ++++++++++++++++
 .../solrj/impl/HttpClusterStateProviderTest.java   | 95 ++++++++++++++++++++++
 4 files changed, 176 insertions(+), 6 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 53aff7c..57d40e3 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -199,6 +199,8 @@ Bug Fixes
 * SOLR-14983: Fix response returning original score instead of reranked score due to query and filter combining.
   (Krishan Goyal, Jason Baik, Christine Poerschke)
 
+* SOLR-14985: Slow indexing and search performance when using HttpClusterStateProvider. (shalin)
+
 Other Changes
 ---------------------
 
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
index ef96be7..106f787 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
@@ -497,9 +497,9 @@ public abstract class BaseCloudSolrClient extends SolrClient {
       throw new SolrServerException("No collection param specified on request and no default collection has been set.");
     }
 
-    //Check to see if the collection is an alias. Updates to multi-collection aliases are ok as long
+    // 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);
+    List<String> aliasedCollections = new ArrayList<>(resolveAliases(Collections.singletonList(collection), false));
     if (getClusterStateProvider().isRoutedAlias(collection) || aliasedCollections.size() == 1) {
       collection = aliasedCollections.get(0); // pick 1st (consistent with HttpSolrCall behavior)
     } else {
@@ -1098,9 +1098,10 @@ public abstract class BaseCloudSolrClient extends SolrClient {
       }
 
     } else if (ADMIN_PATHS.contains(request.getPath())) {
+      // fetch scheme outside the loop because it can be expensive in case of HttpClusterStateProvider
+      String urlScheme = getClusterStateProvider().getClusterProperty(ZkStateReader.URL_SCHEME, "http");
       for (String liveNode : liveNodes) {
-        theUrlList.add(Utils.getBaseUrlForNodeName(liveNode,
-            getClusterStateProvider().getClusterProperty(ZkStateReader.URL_SCHEME,"http")));
+        theUrlList.add(Utils.getBaseUrlForNodeName(liveNode, urlScheme));
       }
 
     } else { // Typical...
@@ -1178,7 +1179,7 @@ public abstract class BaseCloudSolrClient extends SolrClient {
     }
     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 {
@@ -1226,7 +1227,7 @@ public abstract class BaseCloudSolrClient extends SolrClient {
       //no such collection exists
       return null;
     }
-    if (!ref.isLazilyLoaded()) {
+    if (getClusterStateProvider() instanceof ZkClientClusterStateProvider && !ref.isLazilyLoaded()) {
       //it is readily available just return it
       return ref.get();
     }
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CountingHttpClusterStateProvider.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CountingHttpClusterStateProvider.java
new file mode 100644
index 0000000..13fb3bf
--- /dev/null
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CountingHttpClusterStateProvider.java
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.impl;
+
+import org.apache.http.client.HttpClient;
+import org.apache.solr.client.solrj.ResponseParser;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.common.util.NamedList;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+@SuppressWarnings({"unchecked"})
+public class CountingHttpClusterStateProvider extends BaseHttpClusterStateProvider {
+
+  private final HttpClient httpClient;
+  private final boolean clientIsInternal;
+
+  private final AtomicInteger counter = new AtomicInteger(0);
+
+  public CountingHttpClusterStateProvider(List<String> solrUrls, HttpClient httpClient) throws Exception {
+    this.httpClient = httpClient == null ? HttpClientUtil.createClient(null) : httpClient;
+    this.clientIsInternal = httpClient == null;
+    init(solrUrls);
+  }
+
+  @Override
+  protected SolrClient getSolrClient(String baseUrl) {
+    return new AssertingHttpSolrClient(new HttpSolrClient.Builder().withBaseSolrUrl(baseUrl).withHttpClient(httpClient));
+  }
+
+  @Override
+  public void close() throws IOException {
+    if (this.clientIsInternal && this.httpClient != null) {
+      HttpClientUtil.close(httpClient);
+    }
+  }
+
+  public int getRequestCount() {
+    return counter.get();
+  }
+
+  class AssertingHttpSolrClient extends HttpSolrClient {
+    public AssertingHttpSolrClient(Builder builder) {
+      super(builder);
+    }
+
+    @Override
+    public NamedList<Object> request(@SuppressWarnings({"rawtypes"}) SolrRequest request, ResponseParser processor, String collection) throws SolrServerException, IOException {
+      new Exception().printStackTrace();
+      counter.incrementAndGet();
+      return super.request(request, processor, collection);
+    }
+  }
+}
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClusterStateProviderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClusterStateProviderTest.java
new file mode 100644
index 0000000..182b3ab
--- /dev/null
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClusterStateProviderTest.java
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.impl;
+
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrInputDocument;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.List;
+
+public class HttpClusterStateProviderTest extends SolrCloudTestCase {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+  private static final int NODE_COUNT = 1;
+
+  private CloudSolrClient cloudSolrClient;
+  private CountingHttpClusterStateProvider httpClusterStateProvider;
+
+  @Before
+  public void setupCluster() throws Exception {
+    configureCluster(NODE_COUNT)
+      .addConfig("conf", getFile("solrj").toPath().resolve("solr").resolve("configsets").resolve("streaming").resolve("conf"))
+      .configure();
+
+    final List<String> solrUrls = new ArrayList<>();
+    solrUrls.add(cluster.getJettySolrRunner(0).getBaseUrl().toString());
+
+    httpClusterStateProvider = new CountingHttpClusterStateProvider(solrUrls, null);
+    cloudSolrClient = new CloudSolrClient(new CloudSolrClient.Builder(httpClusterStateProvider));
+    CollectionAdminRequest.Create create = CollectionAdminRequest.Create.createCollection("x", 1, 1);
+    CollectionAdminResponse adminResponse = create.process(cloudSolrClient);
+    assertTrue(adminResponse.isSuccess());
+    cloudSolrClient.setDefaultCollection("x");
+  }
+
+  @After
+  public void tearDown() throws Exception {
+    cloudSolrClient.close();
+    httpClusterStateProvider.close();
+    super.tearDown();
+  }
+
+  @Test
+  public void test() throws Exception {
+    // the constructor of HttpClusterStateProvider fetches live nodes
+    // and creating a collection fetches the cluster state
+    // so we can expect exactly 2 http calls to have been made already at this point
+    assertEquals(2, httpClusterStateProvider.getRequestCount());
+
+    QueryResponse queryResponse = cloudSolrClient.query(new SolrQuery("*:*"));
+    assertEquals(0, queryResponse.getResults().getNumFound());
+    // we can expect 1 extra call to fetch and cache the collection state
+    assertEquals(3, httpClusterStateProvider.getRequestCount());
+    queryResponse = cloudSolrClient.query(new SolrQuery("*:*"));
+    assertEquals(0, queryResponse.getResults().getNumFound());
+    // the collection state should already be in the cache so we do not expect another call
+    assertEquals(3, httpClusterStateProvider.getRequestCount());
+
+    cloudSolrClient.add(new SolrInputDocument("id", "a"));
+    // we can expect another call to check if the collection is a routed alias
+    assertEquals(4, httpClusterStateProvider.getRequestCount());
+    cloudSolrClient.add(List.of(new SolrInputDocument("id", "b"), new SolrInputDocument("id", "c")));
+    assertEquals(4, httpClusterStateProvider.getRequestCount());
+    cloudSolrClient.commit();
+    assertEquals(4, httpClusterStateProvider.getRequestCount());
+
+    queryResponse = cloudSolrClient.query(new SolrQuery("*:*"));
+    assertEquals(3, queryResponse.getResults().getNumFound());
+    // the collection state should already be in the cache so we do not expect another call
+    assertEquals(4, httpClusterStateProvider.getRequestCount());
+  }
+}