SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics

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

@@ -198,9 +198,6 @@ public String toString() {
   public final ZkStateReader zkStateReader;
   private SolrCloudManager cloudManager;
 
-  // only for internal usage
-  private Http2SolrClient http2SolrClient;
-
   private CloudHttp2SolrClient cloudSolrClient;
 
   private final String zkServerAddress; // example: 127.0.0.1:54062/solr
@@ -776,7 +773,6 @@ public void close() {
       sysPropsCacher.close();
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudManager));
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudSolrClient));
-      customThreadPool.execute(() -> IOUtils.closeQuietly(http2SolrClient));
 
       try {
         try {
@@ -872,15 +868,14 @@ public SolrCloudManager getSolrCloudManager() {
       if (cloudManager != null) {
         return cloudManager;
       }
-      http2SolrClient =
+      var httpClientBuilder =
           new Http2SolrClient.Builder()
               .withHttpClient(cc.getDefaultHttpSolrClient())
               .withIdleTimeout(30000, TimeUnit.MILLISECONDS)
-              .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
-              .build();
+              .withConnectionTimeout(15000, TimeUnit.MILLISECONDS);
       cloudSolrClient =
           new CloudHttp2SolrClient.Builder(new ZkClientClusterStateProvider(zkStateReader))
-              .withHttpClient(http2SolrClient)
+              .withInternalClientBuilder(httpClientBuilder)
               .build();
       cloudManager = new SolrClientCloudManager(cloudSolrClient, cc.getObjectCache());
       cloudManager.getClusterStateProvider().connect();

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java

@@ -114,7 +114,7 @@ public class HttpShardHandler extends ShardHandler {
   protected AtomicInteger pending;
 
   private final Map<String, List<String>> shardToURLs;
-  protected LBHttp2SolrClient<Http2SolrClient> lbClient;
+  protected LBHttp2SolrClient<Http2SolrClient.Builder> lbClient;
 
   public HttpShardHandler(HttpShardHandlerFactory httpShardHandlerFactory) {
     this.httpShardHandlerFactory = httpShardHandlerFactory;

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java

@@ -84,7 +84,7 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
 
   protected volatile Http2SolrClient defaultClient;
   protected InstrumentedHttpListenerFactory httpListenerFactory;
-  protected LBHttp2SolrClient<Http2SolrClient> loadbalancer;
+  protected LBHttp2SolrClient<Http2SolrClient.Builder> loadbalancer;
 
   int corePoolSize = 0;
   int maximumPoolSize = Integer.MAX_VALUE;
@@ -305,16 +305,17 @@ public void init(PluginInfo info) {
             sb);
     int soTimeout =
         getParameter(args, HttpClientUtil.PROP_SO_TIMEOUT, HttpClientUtil.DEFAULT_SO_TIMEOUT, sb);
-
-    this.defaultClient =
+    var defaultClientBuilder =
         new Http2SolrClient.Builder()
             .withConnectionTimeout(connectionTimeout, TimeUnit.MILLISECONDS)
             .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
             .withExecutor(commExecutor)
-            .withMaxConnectionsPerHost(maxConnectionsPerHost)
-            .build();
+            .withMaxConnectionsPerHost(maxConnectionsPerHost);
+    this.defaultClient = defaultClientBuilder.build();
+
     this.defaultClient.addListenerFactory(this.httpListenerFactory);
-    this.loadbalancer = new LBHttp2SolrClient.Builder<Http2SolrClient>(defaultClient).build();
+    this.loadbalancer =
+        new LBHttp2SolrClient.Builder<Http2SolrClient.Builder>(defaultClientBuilder).build();
 
     initReplicaListTransformers(getParameter(args, "replicaRouting", null, sb));
 

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

@@ -1945,17 +1945,16 @@ public Void answer(InvocationOnMock invocation) {
   }
 
   private SolrCloudManager getCloudDataProvider(ZkStateReader zkStateReader) {
-    var httpSolrClient =
+    var httpSolrClientBuilder =
         new Http2SolrClient.Builder()
             .withIdleTimeout(30000, TimeUnit.MILLISECONDS)
-            .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
-            .build();
+            .withConnectionTimeout(15000, TimeUnit.MILLISECONDS);
     var cloudSolrClient =
         new CloudHttp2SolrClient.Builder(new ZkClientClusterStateProvider(zkStateReader))
-            .withHttpClient(httpSolrClient)
+            .withInternalClientBuilder(httpSolrClientBuilder)
             .build();
     solrClients.add(cloudSolrClient);
-    solrClients.add(httpSolrClient);
+    solrClients.add(httpSolrClientBuilder.build());
     SolrClientCloudManager sccm = new SolrClientCloudManager(cloudSolrClient, null);
     sccm.getClusterStateProvider().connect();
     return sccm;

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java

@@ -40,9 +40,8 @@
 public class CloudHttp2SolrClient extends CloudSolrClient {
 
   private final ClusterStateProvider stateProvider;
-  private final LBHttp2SolrClient<Http2SolrClient> lbClient;
+  private final LBHttp2SolrClient<Http2SolrClient.Builder> lbClient;
   private final Http2SolrClient myClient;
-  private final boolean clientIsInternal;
 
   /**
    * Create a new client object that connects to Zookeeper and is always aware of the SolrCloud
@@ -54,8 +53,8 @@ public class CloudHttp2SolrClient extends CloudSolrClient {
    */
   protected CloudHttp2SolrClient(Builder builder) {
     super(builder.shardLeadersOnly, builder.parallelUpdates, builder.directUpdatesToLeadersOnly);
-    this.clientIsInternal = builder.httpClient == null;
-    this.myClient = createOrGetHttpClientFromBuilder(builder);
+    var httpClientBuilder = createOrGetHttpClientBuilder(builder);
+    this.myClient = httpClientBuilder.build();
     this.stateProvider = createClusterStateProvider(builder);
     this.retryExpiryTimeNano = builder.retryExpiryTimeNano;
     this.defaultCollection = builder.defaultCollection;
@@ -73,16 +72,14 @@ protected CloudHttp2SolrClient(Builder builder) {
     // locks.
     this.locks = objectList(builder.parallelCacheRefreshesLocks);
 
-    this.lbClient = new LBHttp2SolrClient.Builder<Http2SolrClient>(myClient).build();
+    this.lbClient = new LBHttp2SolrClient.Builder<>(httpClientBuilder).build();
   }
 
-  private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
-    if (builder.httpClient != null) {
-      return builder.httpClient;
-    } else if (builder.internalClientBuilder != null) {
-      return builder.internalClientBuilder.build();
+  private Http2SolrClient.Builder createOrGetHttpClientBuilder(Builder builder) {
+    if (builder.internalClientBuilder != null) {
+      return builder.internalClientBuilder;
     } else {
-      return new Http2SolrClient.Builder().build();
+      return new Http2SolrClient.Builder();
     }
   }
 
@@ -129,7 +126,7 @@ private ClusterStateProvider createHttp2ClusterStateProvider(
 
   private void closeMyClientIfNeeded() {
     try {
-      if (clientIsInternal && myClient != null) {
+      if (myClient != null) {
         myClient.close();
       }
     } catch (Exception e) {
@@ -148,7 +145,7 @@ public void close() throws IOException {
   }
 
   @Override
-  public LBHttp2SolrClient<Http2SolrClient> getLbClient() {
+  public LBHttp2SolrClient<Http2SolrClient.Builder> getLbClient() {
     return lbClient;
   }
 
@@ -171,7 +168,6 @@ public static class Builder {
     protected Collection<String> zkHosts = new ArrayList<>();
     protected List<String> solrUrls = new ArrayList<>();
     protected String zkChroot;
-    protected Http2SolrClient httpClient;
     protected boolean shardLeadersOnly = true;
     protected boolean directUpdatesToLeadersOnly = false;
     protected boolean parallelUpdates = true;
@@ -404,23 +400,6 @@ public Builder withCollectionCacheTtl(long timeToLive, TimeUnit unit) {
       return this;
     }
 
-    /**
-     * Set the internal http client.
-     *
-     * <p>Note: closing the httpClient instance is at the responsibility of the caller.
-     *
-     * @param httpClient http client
-     * @return this
-     */
-    public Builder withHttpClient(Http2SolrClient httpClient) {
-      if (this.internalClientBuilder != null) {
-        throw new IllegalStateException(
-            "The builder can't accept an httpClient AND an internalClientBuilder, only one of those can be provided");
-      }
-      this.httpClient = httpClient;
-      return this;
-    }
-
     /**
      * If provided, the CloudHttp2SolrClient will build it's internal Http2SolrClient using this
      * builder (instead of the empty default one). Providing this builder allows users to configure
@@ -430,10 +409,6 @@ public Builder withHttpClient(Http2SolrClient httpClient) {
      * @return this
      */
     public Builder withInternalClientBuilder(Http2SolrClient.Builder internalClientBuilder) {
-      if (this.httpClient != null) {
-        throw new IllegalStateException(
-            "The builder can't accept an httpClient AND an internalClientBuilder, only one of those can be provided");
-      }
       this.internalClientBuilder = internalClientBuilder;
       return this;
     }

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java

@@ -138,7 +138,7 @@ protected HttpJdkSolrClient(String serverBaseUrl, HttpJdkSolrClient.Builder buil
   public CompletableFuture<NamedList<Object>> requestAsync(
       final SolrRequest<?> solrRequest, String collection) {
     try {
-      PreparedRequest pReq = prepareRequest(solrRequest, collection, null);
+      PreparedRequest pReq = prepareRequest(solrRequest, collection);
       return httpClient
           .sendAsync(pReq.reqb.build(), HttpResponse.BodyHandlers.ofInputStream())
           .thenApply(
@@ -157,10 +157,10 @@ public CompletableFuture<NamedList<Object>> requestAsync(
     }
   }
 
-  protected NamedList<Object> requestWithBaseUrl(
-      String baseUrl, SolrRequest<?> solrRequest, String collection)
+  @Override
+  public NamedList<Object> request(SolrRequest<?> solrRequest, String collection)
       throws SolrServerException, IOException {
-    PreparedRequest pReq = prepareRequest(solrRequest, collection, baseUrl);
+    PreparedRequest pReq = prepareRequest(solrRequest, collection);
     HttpResponse<InputStream> response = null;
     try {
       response = httpClient.send(pReq.reqb.build(), HttpResponse.BodyHandlers.ofInputStream());
@@ -192,25 +192,13 @@ protected NamedList<Object> requestWithBaseUrl(
     }
   }
 
-  @Override
-  public NamedList<Object> request(SolrRequest<?> solrRequest, String collection)
-      throws SolrServerException, IOException {
-    return requestWithBaseUrl(null, solrRequest, collection);
-  }
-
-  private PreparedRequest prepareRequest(
-      SolrRequest<?> solrRequest, String collection, String overrideBaseUrl)
+  private PreparedRequest prepareRequest(SolrRequest<?> solrRequest, String collection)
       throws SolrServerException, IOException {
     checkClosed();
     if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) {
       collection = defaultCollection;
     }
-    String url;
-    if (overrideBaseUrl != null) {
-      url = ClientUtils.buildRequestUrl(solrRequest, overrideBaseUrl, collection);
-    } else {
-      url = getRequestUrl(solrRequest, collection);
-    }
+    String url = getRequestUrl(solrRequest, collection);
     ResponseParser parserToUse = responseParser(solrRequest);
     ModifiableSolrParams queryParams = initializeSolrParams(solrRequest, parserToUse);
     var reqb = HttpRequest.newBuilder();