SOLR-17874: Stop using HttpSolrClient in production (#3501)

Switch remaining usages of Apache HttpClient to use the internally managed Jetty HttpClient instance.
* shard split: the commits
* SYNCSHARD command/action
* admin handler (e.g. log level change with nodes param)
* IterativeMergeStrategy
* config edit API
* schema edit API
* NodesSysPropsCacher: use default client not that of shardHandler
Still using it in tests.

And removed some deprecated internal options to use Apache HttpClient

Author: dsmiley

solr/CHANGES.txt

@@ -213,6 +213,8 @@ Other Changes
 
 * SOLR-17286: When proxying requests to another node, use Jetty HttpClient not Apache HttpClient. (David Smiley)
 
+* SOLR-17874: Switch remaining usages of Apache HttpClient to use the internally managed Jetty HttpClient instance. (David Smiley)
+
 ==================  9.10.0 ==================
 New Features
 ---------------------

solr/benchmark/build.gradle

@@ -46,7 +46,6 @@ dependencies {
   implementation project(':solr:solrj-streaming')
 
   implementation libs.apache.lucene.core
-  implementation libs.apache.httpcomponents.httpclient
   implementation libs.commonsio.commonsio
   implementation libs.dropwizard.metrics.core
   implementation libs.apache.commons.math3

solr/benchmark/gradle.lockfile

@@ -32,7 +32,7 @@ com.jayway.jsonpath:json-path:2.9.0=jarValidation,runtimeClasspath,testRuntimeCl
 com.lmax:disruptor:3.4.4=jarValidation,runtimeClasspath,testRuntimeClasspath
 com.tdunning:t-digest:3.3=jarValidation,runtimeClasspath,testRuntimeClasspath
 commons-cli:commons-cli:1.9.0=jarValidation,runtimeClasspath,testRuntimeClasspath
-commons-codec:commons-codec:1.17.2=compileClasspath,jarValidation,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
+commons-codec:commons-codec:1.17.2=jarValidation,runtimeClasspath,testRuntimeClasspath
 commons-io:commons-io:2.17.0=compileClasspath,jarValidation,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 io.dropwizard.metrics:metrics-annotation:4.2.26=jarValidation,runtimeClasspath,testRuntimeClasspath
 io.dropwizard.metrics:metrics-core:4.2.26=compileClasspath,jarValidation,runtimeClasspath,testCompileClasspath,testRuntimeClasspath

solr/benchmark/src/java/org/apache/solr/bench/search/StreamingSearch.java

@@ -28,7 +28,6 @@
 import org.apache.solr.bench.MiniClusterState.MiniClusterBenchState;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.Http2SolrClient;
-import org.apache.solr.client.solrj.impl.HttpClientUtil;
 import org.apache.solr.client.solrj.io.SolrClientCache;
 import org.apache.solr.client.solrj.io.Tuple;
 import org.apache.solr.client.solrj.io.stream.CloudSolrStream;
@@ -98,13 +97,9 @@ public void setup(MiniClusterBenchState miniClusterState) throws Exception {
     public void setupIteration(MiniClusterState.MiniClusterBenchState miniClusterState)
         throws SolrServerException, IOException {
       SolrClientCache solrClientCache;
-      if (useHttp1) {
-        var httpClient = HttpClientUtil.createClient(null); // TODO tune params?
-        solrClientCache = new SolrClientCache(httpClient);
-      } else {
-        http2SolrClient = newHttp2SolrClient();
-        solrClientCache = new SolrClientCache(http2SolrClient);
-      }
+      // TODO tune params?
+      var client = new Http2SolrClient.Builder().useHttp1_1(useHttp1).build();
+      solrClientCache = new SolrClientCache(client);
 
       streamContext = new StreamContext();
       streamContext.setSolrClientCache(solrClientCache);
@@ -145,10 +140,4 @@ private static List<Tuple> getTuples(TupleStream tupleStream) throws IOException
       tupleStream.close();
     }
   }
-
-  public static Http2SolrClient newHttp2SolrClient() {
-    // TODO tune params?
-    var builder = new Http2SolrClient.Builder();
-    return builder.build();
-  }
 }

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

@@ -61,7 +61,6 @@
 import org.apache.solr.client.solrj.cloud.SolrCloudManager;
 import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
 import org.apache.solr.client.solrj.impl.CloudSolrClient;
-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;
@@ -118,7 +117,6 @@
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.SolrCoreInitializationException;
 import org.apache.solr.handler.component.HttpShardHandler;
-import org.apache.solr.handler.component.HttpShardHandlerFactory;
 import org.apache.solr.logging.MDCLoggingContext;
 import org.apache.solr.search.SolrIndexSearcher;
 import org.apache.solr.update.UpdateLog;
@@ -393,10 +391,7 @@ public ZkController(
     }
     this.overseerCollectionQueue = overseer.getCollectionQueue(zkClient);
     this.overseerConfigSetQueue = overseer.getConfigSetQueue(zkClient);
-    final var client =
-        (Http2SolrClient)
-            ((HttpShardHandlerFactory) getCoreContainer().getShardHandlerFactory()).getClient();
-    this.sysPropsCacher = new NodesSysPropsCacher(client, zkStateReader);
+    this.sysPropsCacher = new NodesSysPropsCacher(cc.getDefaultHttpSolrClient(), zkStateReader);
     assert ObjectReleaseTracker.track(this);
   }
 

solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java

@@ -39,7 +39,7 @@
 import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.SolrServerException;
-import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.client.solrj.request.AbstractUpdateRequest;
 import org.apache.solr.client.solrj.request.UpdateRequest;
 import org.apache.solr.client.solrj.response.UpdateResponse;
@@ -218,14 +218,19 @@ static void checkResults(String label, NamedList<Object> results, boolean failur
     }
   }
 
-  static void commit(NamedList<Object> results, String slice, Replica parentShardLeader) {
+  static void commit(
+      Http2SolrClient solrClient,
+      NamedList<Object> results,
+      String slice,
+      Replica parentShardLeader) {
     log.debug("Calling soft commit to make sub shard updates visible");
     String coreUrl = parentShardLeader.getCoreUrl();
     // HttpShardHandler is hard coded to send a QueryRequest hence we go direct
     // and we force open a searcher so that we have documents to show upon switching states
     UpdateResponse updateResponse = null;
     try {
-      updateResponse = softCommit(parentShardLeader.getBaseUrl(), parentShardLeader.getCoreName());
+      updateResponse =
+          softCommit(solrClient, parentShardLeader.getBaseUrl(), parentShardLeader.getCoreName());
       CollectionHandlingUtils.processResponse(
           results, null, coreUrl, updateResponse, slice, Collections.emptySet());
     } catch (Exception e) {
@@ -238,19 +243,12 @@ static void commit(NamedList<Object> results, String slice, Replica parentShardL
     }
   }
 
-  static UpdateResponse softCommit(String baseUrl, String coreName)
+  private static UpdateResponse softCommit(
+      Http2SolrClient solrClient, String baseUrl, String coreName)
       throws SolrServerException, IOException {
-
-    try (SolrClient client =
-        new HttpSolrClient.Builder(baseUrl)
-            .withDefaultCollection(coreName)
-            .withConnectionTimeout(30000, TimeUnit.MILLISECONDS)
-            .withSocketTimeout(120000, TimeUnit.MILLISECONDS)
-            .build()) {
-      UpdateRequest ureq = new UpdateRequest();
-      ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, false, true, true);
-      return ureq.process(client);
-    }
+    UpdateRequest ureq = new UpdateRequest();
+    ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, false, true, true);
+    return solrClient.requestWithBaseUrl(baseUrl, coreName, ureq);
   }
 
   public static String waitForCoreNodeName(

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

@@ -716,7 +716,11 @@ public boolean split(ClusterState clusterState, ZkNodeProps message, NamedList<O
         // state switch as per SOLR-13945 so that sub shards don't come up empty, momentarily, after
         // being marked active)
         t = timings.sub("finalCommit");
-        CollectionHandlingUtils.commit(results, slice.get(), parentShardLeader);
+        CollectionHandlingUtils.commit(
+            ccc.getCoreContainer().getUpdateShardHandler().getUpdateOnlyHttpClient(),
+            results,
+            slice.get(),
+            parentShardLeader);
         t.stop();
         // switch sub shard states to 'active'
         log.info("Replication factor is 1 so switching shard states");
@@ -796,7 +800,11 @@ public boolean split(ClusterState clusterState, ZkNodeProps message, NamedList<O
       // when the sub-shard replicas come up
       if (repFactor > 1) {
         t = timings.sub("finalCommit");
-        CollectionHandlingUtils.commit(results, slice.get(), parentShardLeader);
+        CollectionHandlingUtils.commit(
+            ccc.getCoreContainer().getUpdateShardHandler().getUpdateOnlyHttpClient(),
+            results,
+            slice.get(),
+            parentShardLeader);
         t.stop();
       }
 

solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java

@@ -55,9 +55,10 @@
 import org.apache.solr.api.Api;
 import org.apache.solr.api.ApiBag;
 import org.apache.solr.client.solrj.SolrResponse;
-import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.client.solrj.io.stream.expr.Expressible;
 import org.apache.solr.client.solrj.request.CollectionRequiringSolrRequest;
+import org.apache.solr.client.solrj.response.SimpleSolrResponse;
 import org.apache.solr.cloud.ZkController;
 import org.apache.solr.cloud.ZkSolrResourceLoader;
 import org.apache.solr.common.MapSerializable;
@@ -843,8 +844,10 @@ private static void waitForAllReplicasState(
     // course)
     List<PerReplicaCallable> concurrentTasks = new ArrayList<>();
 
+    var http2SolrClient = zkController.getCoreContainer().getDefaultHttpSolrClient();
     for (Replica replica : getActiveReplicas(zkController, collection)) {
-      PerReplicaCallable e = new PerReplicaCallable(replica, prop, expectedVersion, maxWaitSecs);
+      PerReplicaCallable e =
+          new PerReplicaCallable(http2SolrClient, replica, prop, expectedVersion, maxWaitSecs);
       concurrentTasks.add(e);
     }
     if (concurrentTasks.isEmpty()) return; // nothing to wait for ...
@@ -857,6 +860,7 @@ private static void waitForAllReplicasState(
     }
 
     // use an executor service to invoke schema zk version requests in parallel with a max wait time
+    // TODO use httpSolrClient.requestAsync instead; it has an executor
     int poolSize = Math.min(concurrentTasks.size(), 10);
     ExecutorService parallelExecutor =
         ExecutorUtil.newMDCAwareFixedThreadPool(
@@ -959,14 +963,21 @@ public Name getPermissionName(AuthorizationContext ctx) {
 
   private static class PerReplicaCallable extends CollectionRequiringSolrRequest<SolrResponse>
       implements Callable<Boolean> {
+    private final Http2SolrClient solrClient;
     Replica replica;
     String prop;
     int expectedZkVersion;
     Number remoteVersion = null;
     int maxWait;
 
-    PerReplicaCallable(Replica replica, String prop, int expectedZkVersion, int maxWait) {
+    PerReplicaCallable(
+        Http2SolrClient solrClient,
+        Replica replica,
+        String prop,
+        int expectedZkVersion,
+        int maxWait) {
       super(METHOD.GET, "/config/" + ZNODEVER, SolrRequestType.ADMIN);
+      this.solrClient = solrClient;
       this.replica = replica;
       this.expectedZkVersion = expectedZkVersion;
       this.prop = prop;
@@ -984,42 +995,41 @@ public SolrParams getParams() {
     public Boolean call() throws Exception {
       final RTimer timer = new RTimer();
       int attempts = 0;
-      try (HttpSolrClient solr =
-          new HttpSolrClient.Builder(replica.getBaseUrl())
-              .withDefaultCollection(replica.getCoreName())
-              .build()) {
-        // eventually, this loop will get killed by the ExecutorService's timeout
-        while (true) {
-          try {
-            long timeElapsed = (long) timer.getTime() / 1000;
-            if (timeElapsed >= maxWait) {
-              return false;
-            }
-            log.info("Time elapsed : {} secs, maxWait {}", timeElapsed, maxWait);
-            Thread.sleep(100);
-            NamedList<Object> resp = solr.httpUriRequest(this).future.get();
-            if (resp != null) {
-              @SuppressWarnings({"rawtypes"})
-              Map m = (Map) resp.get(ZNODEVER);
-              if (m != null) {
-                remoteVersion = (Number) m.get(prop);
-                if (remoteVersion != null && remoteVersion.intValue() >= expectedZkVersion) break;
-              }
+      // eventually, this loop will get killed by the ExecutorService's timeout
+      while (true) {
+        try {
+          long timeElapsed = (long) timer.getTime() / 1000;
+          if (timeElapsed >= maxWait) {
+            return false;
+          }
+          log.info("Time elapsed : {} secs, maxWait {}", timeElapsed, maxWait);
+          Thread.sleep(100);
+
+          NamedList<Object> resp =
+              solrClient
+                  .requestWithBaseUrl(replica.getBaseUrl(), replica.getCoreName(), this)
+                  .getResponse();
+          if (resp != null) {
+            @SuppressWarnings({"rawtypes"})
+            Map m = (Map) resp.get(ZNODEVER);
+            if (m != null) {
+              remoteVersion = (Number) m.get(prop);
+              if (remoteVersion != null && remoteVersion.intValue() >= expectedZkVersion) break;
             }
+          }
 
-            attempts++;
-            if (log.isInfoEnabled()) {
-              log.info(
-                  formatString(
-                      "Could not get expectedVersion {0} from {1} for prop {2}   after {3} attempts",
-                      expectedZkVersion, replica.getCoreUrl(), prop, attempts));
-            }
-          } catch (Exception e) {
-            if (e instanceof InterruptedException) {
-              break; // stop looping
-            } else {
-              log.warn("Failed to get /schema/zkversion from {} due to: ", replica.getCoreUrl(), e);
-            }
+          attempts++;
+          if (log.isInfoEnabled()) {
+            log.info(
+                formatString(
+                    "Could not get expectedVersion {0} from {1} for prop {2}   after {3} attempts",
+                    expectedZkVersion, replica.getCoreUrl(), prop, attempts));
+          }
+        } catch (Exception e) {
+          if (e instanceof InterruptedException) {
+            break; // stop looping
+          } else {
+            log.warn("Failed to get /schema/zkversion from {} due to: ", replica.getCoreUrl(), e);
           }
         }
       }
@@ -1028,7 +1038,7 @@ public Boolean call() throws Exception {
 
     @Override
     protected SolrResponse createResponse(NamedList<Object> namedList) {
-      return null;
+      return new SimpleSolrResponse();
     }
   }
 

solr/core/src/java/org/apache/solr/handler/admin/AdminHandlersProxy.java

@@ -21,25 +21,23 @@
 import java.lang.invoke.MethodHandles;
 import java.net.URI;
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-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.client.solrj.impl.HttpSolrClient;
 import org.apache.solr.client.solrj.request.GenericSolrRequest;
 import org.apache.solr.cloud.ZkController;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
-import org.apache.solr.common.util.Pair;
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
@@ -101,16 +99,14 @@ public static boolean maybeProxyToNodes(
 
     ModifiableSolrParams params = new ModifiableSolrParams(req.getParams());
     params.remove(PARAM_NODES);
-    Map<String, Pair<Future<NamedList<Object>>, SolrClient>> responses = new HashMap<>();
+    Map<String, Future<NamedList<Object>>> responses = new LinkedHashMap<>();
     for (String node : nodes) {
       responses.put(node, callRemoteNode(node, pathStr, params, container.getZkController()));
     }
 
-    for (Map.Entry<String, Pair<Future<NamedList<Object>>, SolrClient>> entry :
-        responses.entrySet()) {
+    for (Map.Entry<String, Future<NamedList<Object>>> entry : responses.entrySet()) {
       try {
-        NamedList<Object> resp = entry.getValue().first().get(10, TimeUnit.SECONDS);
-        entry.getValue().second().close();
+        NamedList<Object> resp = entry.getValue().get(10, TimeUnit.SECONDS);
         rsp.add(entry.getKey(), resp);
       } catch (ExecutionException ee) {
         log.warn("Exception when fetching result from node {}", entry.getKey(), ee);
@@ -125,18 +121,17 @@ public static boolean maybeProxyToNodes(
     return true;
   }
 
-  /**
-   * Makes a remote request and returns a future and the solr client. The caller is responsible for
-   * closing the client
-   */
-  public static Pair<Future<NamedList<Object>>, SolrClient> callRemoteNode(
-      String nodeName, String endpoint, SolrParams params, ZkController zkController)
+  /** Makes a remote request asynchronously. */
+  public static CompletableFuture<NamedList<Object>> callRemoteNode(
+      String nodeName, String uriPath, SolrParams params, ZkController zkController)
       throws IOException, SolrServerException {
-    log.debug("Proxying {} request to node {}", endpoint, nodeName);
+    log.debug("Proxying {} request to node {}", uriPath, nodeName);
     URI baseUri = URI.create(zkController.zkStateReader.getBaseUrlForNodeName(nodeName));
-    HttpSolrClient solr = new HttpSolrClient.Builder(baseUri.toString()).build();
-    SolrRequest<?> proxyReq = new GenericSolrRequest(SolrRequest.METHOD.GET, endpoint, params);
-    HttpSolrClient.HttpUriRequestResponse proxyResp = solr.httpUriRequest(proxyReq);
-    return new Pair<>(proxyResp.future, solr);
+    SolrRequest<?> proxyReq = new GenericSolrRequest(SolrRequest.METHOD.GET, uriPath, params);
+
+    return zkController
+        .getCoreContainer()
+        .getDefaultHttpSolrClient()
+        .requestWithBaseUrl(baseUri.toString(), c -> c.requestAsync(proxyReq));
   }
 }