Switched from HTTP1 to HTTP2 in SolrClientCloudManager (#2751)

* Replaced CloudLegacySolrClient with CloudHttp2SolrClient in SolrClientCloudManager and updated all internal classes accordingly.
* SolrCloudManager#httpRequest is removed.
* Removed dependencies on Apache httpclient and httpcore from solrj-zookeeper

---------

Co-authored-by: David Smiley <dsmiley@apache.org>
(cherry picked from commit d0000b4)

solr/CHANGES.txt

@@ -64,6 +64,8 @@ Optimizations
 
 * SOLR-17441: Improve system metrics collection by skipping unreadable MXBean properties, making /admin/info/system calls faster (Haythem Khiri)
 
+* SOLR-16503: Switched from HTTP1 to HTTP2 in SolrClientCloudManager by replacing CloudLegacySolrClient with CloudHttp2SolrClient. (Sanjay Dutt, David Smiley)
+
 Bug Fixes
 ---------------------
 * SOLR-12429: Uploading a configset with a symbolic link produces a IOException. Now a error message to user generated instead. (Eric Pugh)

solr/core/src/java/org/apache/solr/cloud/ZkController.java

@@ -58,11 +58,11 @@
 import java.util.stream.Collectors;
 import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.cloud.SolrCloudManager;
-import org.apache.solr.client.solrj.impl.CloudLegacySolrClient;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.client.solrj.impl.HttpSolrClient.Builder;
 import org.apache.solr.client.solrj.impl.SolrClientCloudManager;
 import org.apache.solr.client.solrj.impl.SolrZkClientTimeout;
-import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
 import org.apache.solr.client.solrj.request.CoreAdminRequest.WaitForState;
 import org.apache.solr.cloud.overseer.ClusterStateMutator;
 import org.apache.solr.cloud.overseer.OverseerAction;
@@ -204,7 +204,11 @@ public String toString() {
   private final SolrZkClient zkClient;
   public final ZkStateReader zkStateReader;
   private SolrCloudManager cloudManager;
-  private CloudLegacySolrClient cloudSolrClient;
+
+  // only for internal usage
+  private Http2SolrClient http2SolrClient;
+
+  private CloudHttp2SolrClient cloudSolrClient;
 
   private final String zkServerAddress; // example: 127.0.0.1:54062/solr
 
@@ -772,8 +776,9 @@ public void close() {
     } finally {
 
       sysPropsCacher.close();
-      customThreadPool.execute(() -> IOUtils.closeQuietly(cloudSolrClient));
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudManager));
+      customThreadPool.execute(() -> IOUtils.closeQuietly(cloudSolrClient));
+      customThreadPool.execute(() -> IOUtils.closeQuietly(http2SolrClient));
 
       try {
         try {
@@ -869,11 +874,15 @@ public SolrCloudManager getSolrCloudManager() {
       if (cloudManager != null) {
         return cloudManager;
       }
-      cloudSolrClient =
-          new CloudLegacySolrClient.Builder(new ZkClientClusterStateProvider(zkStateReader))
-              .withHttpClient(cc.getUpdateShardHandler().getDefaultHttpClient())
+      http2SolrClient =
+          new Http2SolrClient.Builder()
+              .withHttpClient(cc.getDefaultHttpSolrClient())
+              .withIdleTimeout(30000, TimeUnit.MILLISECONDS)
               .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
-              .withSocketTimeout(30000, TimeUnit.MILLISECONDS)
+              .build();
+      cloudSolrClient =
+          new CloudHttp2SolrClient.Builder(List.of(getZkServerAddress()), Optional.empty())
+              .withHttpClient(http2SolrClient)
               .build();
       cloudManager = new SolrClientCloudManager(cloudSolrClient, cc.getObjectCache());
       cloudManager.getClusterStateProvider().connect();

solr/core/src/test/org/apache/solr/cloud/OverseerTest.java

@@ -52,8 +52,8 @@
 import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.cloud.DistributedQueue;
 import org.apache.solr.client.solrj.cloud.SolrCloudManager;
-import org.apache.solr.client.solrj.impl.CloudLegacySolrClient;
-import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.client.solrj.impl.SolrClientCloudManager;
 import org.apache.solr.cloud.overseer.NodeMutator;
 import org.apache.solr.cloud.overseer.OverseerAction;
@@ -127,7 +127,7 @@ public class OverseerTest extends SolrTestCaseJ4 {
       Collections.synchronizedList(new ArrayList<>());
   private final List<UpdateShardHandler> updateShardHandlers =
       Collections.synchronizedList(new ArrayList<>());
-  private final List<CloudSolrClient> solrClients = Collections.synchronizedList(new ArrayList<>());
+  private final List<SolrClient> solrClients = Collections.synchronizedList(new ArrayList<>());
   private static final String COLLECTION = SolrTestCaseJ4.DEFAULT_TEST_COLLECTION_NAME;
 
   public static class MockZKController {
@@ -1949,13 +1949,18 @@ public Void answer(InvocationOnMock invocation) {
   }
 
   private SolrCloudManager getCloudDataProvider(String zkAddress) {
-    var client =
-        new CloudLegacySolrClient.Builder(Collections.singletonList(zkAddress), Optional.empty())
-            .withSocketTimeout(30000, TimeUnit.MILLISECONDS)
+    var httpSolrClient =
+        new Http2SolrClient.Builder()
+            .withIdleTimeout(30000, TimeUnit.MILLISECONDS)
             .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
             .build();
-    solrClients.add(client);
-    SolrClientCloudManager sccm = new SolrClientCloudManager(client);
+    var cloudSolrClient =
+        new CloudHttp2SolrClient.Builder(Collections.singletonList(zkAddress), Optional.empty())
+            .withHttpClient(httpSolrClient)
+            .build();
+    solrClients.add(cloudSolrClient);
+    solrClients.add(httpSolrClient);
+    SolrClientCloudManager sccm = new SolrClientCloudManager(cloudSolrClient, null);
     sccm.getClusterStateProvider().connect();
     return sccm;
   }

solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java

@@ -36,6 +36,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.common.MapWriter;
 import org.apache.solr.common.cloud.ClusterProperties;
 import org.apache.solr.common.cloud.ClusterState;
@@ -551,6 +552,7 @@ public void tearDown() throws Exception {
   private static class MockCoreContainer extends CoreContainer {
     UpdateShardHandler updateShardHandler =
         new UpdateShardHandler(UpdateShardHandlerConfig.DEFAULT);
+    Http2SolrClient solrClient;
 
     public MockCoreContainer() {
       super(SolrXmlConfig.fromString(TEST_PATH(), "<solr/>"));
@@ -559,6 +561,7 @@ public MockCoreContainer() {
       this.shardHandlerFactory = httpShardHandlerFactory;
       this.coreAdminHandler = new CoreAdminHandler();
       this.metricManager = mock(SolrMetricManager.class);
+      this.solrClient = new Http2SolrClient.Builder().build();
     }
 
     @Override
@@ -572,9 +575,15 @@ public UpdateShardHandler getUpdateShardHandler() {
     @Override
     public void shutdown() {
       updateShardHandler.close();
+      solrClient.close();
       super.shutdown();
     }
 
+    @Override
+    public Http2SolrClient getDefaultHttpSolrClient() {
+      return solrClient;
+    }
+
     @Override
     public SolrMetricManager getMetricManager() {
       return metricManager;

solr/solrj-zookeeper/build.gradle

@@ -34,8 +34,6 @@ dependencies {
 
     // declare dependencies we use even though already declared by solrj-core
     implementation 'org.slf4j:slf4j-api'
-    implementation 'org.apache.httpcomponents:httpclient'
-    implementation 'org.apache.httpcomponents:httpcore'
 
     api('org.apache.zookeeper:zookeeper', {
         exclude group: "org.apache.yetus", module: "audience-annotations"

solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/cloud/DelegatingCloudManager.java

@@ -17,7 +17,6 @@
 package org.apache.solr.client.solrj.cloud;
 
 import java.io.IOException;
-import java.util.Map;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.impl.ClusterStateProvider;
@@ -69,18 +68,6 @@ public <T extends SolrResponse> T request(SolrRequest<T> req) throws IOException
     return delegate.request(req);
   }
 
-  @Override
-  public byte[] httpRequest(
-      String url,
-      SolrRequest.METHOD method,
-      Map<String, String> headers,
-      String payload,
-      int timeout,
-      boolean followRedirects)
-      throws IOException {
-    return delegate.httpRequest(url, method, headers, payload, timeout, followRedirects);
-  }
-
   @Override
   public void close() throws IOException {
     delegate.close();

solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/cloud/SolrCloudManager.java

@@ -18,7 +18,6 @@
 package org.apache.solr.client.solrj.cloud;
 
 import java.io.IOException;
-import java.util.Map;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.impl.ClusterStateProvider;
@@ -53,13 +52,4 @@ default ClusterState getClusterState() throws IOException {
   // Solr-like methods
 
   <T extends SolrResponse> T request(SolrRequest<T> req) throws IOException;
-
-  byte[] httpRequest(
-      String url,
-      SolrRequest.METHOD method,
-      Map<String, String> headers,
-      String payload,
-      int timeout,
-      boolean followRedirects)
-      throws IOException;
 }

solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientCloudManager.java

@@ -19,20 +19,6 @@
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
-import org.apache.http.HttpEntity;
-import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
-import org.apache.http.client.config.RequestConfig;
-import org.apache.http.client.methods.HttpDelete;
-import org.apache.http.client.methods.HttpGet;
-import org.apache.http.client.methods.HttpPost;
-import org.apache.http.client.methods.HttpPut;
-import org.apache.http.client.methods.HttpRequestBase;
-import org.apache.http.client.protocol.HttpClientContext;
-import org.apache.http.entity.StringEntity;
-import org.apache.http.util.EntityUtils;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.SolrServerException;
@@ -51,21 +37,17 @@
 public class SolrClientCloudManager implements SolrCloudManager {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
-  protected final CloudLegacySolrClient solrClient;
+  private final CloudHttp2SolrClient cloudSolrClient;
   private final ZkDistribStateManager stateManager;
   private final ZkStateReader zkStateReader;
   private final SolrZkClient zkClient;
   private final ObjectCache objectCache;
   private final boolean closeObjectCache;
   private volatile boolean isClosed;
 
-  public SolrClientCloudManager(CloudLegacySolrClient solrClient) {
-    this(solrClient, null);
-  }
-
-  public SolrClientCloudManager(CloudLegacySolrClient solrClient, ObjectCache objectCache) {
-    this.solrClient = solrClient;
-    this.zkStateReader = ZkStateReader.from(solrClient);
+  public SolrClientCloudManager(CloudHttp2SolrClient client, ObjectCache objectCache) {
+    this.cloudSolrClient = client;
+    this.zkStateReader = ZkStateReader.from(client);
     this.zkClient = zkStateReader.getZkClient();
     this.stateManager = new ZkDistribStateManager(zkClient);
     this.isClosed = false;
@@ -103,12 +85,12 @@ public TimeSource getTimeSource() {
 
   @Override
   public ClusterStateProvider getClusterStateProvider() {
-    return solrClient.getClusterStateProvider();
+    return cloudSolrClient.getClusterStateProvider();
   }
 
   @Override
   public NodeStateProvider getNodeStateProvider() {
-    return new SolrClientNodeStateProvider(solrClient);
+    return new SolrClientNodeStateProvider(cloudSolrClient);
   }
 
   @Override
@@ -119,76 +101,14 @@ public DistribStateManager getDistribStateManager() {
   @Override
   public <T extends SolrResponse> T request(SolrRequest<T> req) throws IOException {
     try {
-      return req.process(solrClient);
+      return req.process(cloudSolrClient);
     } catch (SolrServerException e) {
       throw new IOException(e);
     }
   }
 
   private static final byte[] EMPTY = new byte[0];
 
-  @Override
-  public byte[] httpRequest(
-      String url,
-      SolrRequest.METHOD method,
-      Map<String, String> headers,
-      String payload,
-      int timeout,
-      boolean followRedirects)
-      throws IOException {
-    HttpClient client = solrClient.getHttpClient();
-    final HttpRequestBase req;
-    HttpEntity entity = null;
-    if (payload != null) {
-      entity = new StringEntity(payload, StandardCharsets.UTF_8);
-    }
-    switch (method) {
-      case GET:
-        req = new HttpGet(url);
-        break;
-      case POST:
-        req = new HttpPost(url);
-        if (entity != null) {
-          ((HttpPost) req).setEntity(entity);
-        }
-        break;
-      case PUT:
-        req = new HttpPut(url);
-        if (entity != null) {
-          ((HttpPut) req).setEntity(entity);
-        }
-        break;
-      case DELETE:
-        req = new HttpDelete(url);
-        break;
-      default:
-        throw new IOException("Unsupported method " + method);
-    }
-    if (headers != null) {
-      headers.forEach((k, v) -> req.addHeader(k, v));
-    }
-    RequestConfig.Builder requestConfigBuilder = HttpClientUtil.createDefaultRequestConfigBuilder();
-    if (timeout > 0) {
-      requestConfigBuilder.setSocketTimeout(timeout);
-      requestConfigBuilder.setConnectTimeout(timeout);
-    }
-    requestConfigBuilder.setRedirectsEnabled(followRedirects);
-    req.setConfig(requestConfigBuilder.build());
-    HttpClientContext httpClientRequestContext = HttpClientUtil.createNewHttpClientRequestContext();
-    HttpResponse rsp = client.execute(req, httpClientRequestContext);
-    int statusCode = rsp.getStatusLine().getStatusCode();
-    if (statusCode != 200) {
-      throw new IOException(
-          "Error sending request to " + url + ", HTTP response: " + rsp.toString());
-    }
-    HttpEntity responseEntity = rsp.getEntity();
-    if (responseEntity != null && responseEntity.getContent() != null) {
-      return EntityUtils.toByteArray(responseEntity);
-    } else {
-      return EMPTY;
-    }
-  }
-
   public SolrZkClient getZkClient() {
     return zkClient;
   }