Revert "SOLR-17541: LBSolrClient implementations should agree on 'get…

…Client()' semantics (#2899)"

This reverts commit d3e57aa.

solr/CHANGES.txt

@@ -34,10 +34,6 @@ Improvements
 
 * SOLR-17516: `LBHttp2SolrClient` is now generic, adding support for `HttpJdkSolrClient`. (James Dyer)
 
-* SOLR-17541: `LBHttp2SolrClient` now maintains a separate internal/delegate client per Solr Base URL.  Both `LBHttp2SolrClient` and `CloudHttp2SolrClient`
-  always create and manage these internal clients.  The ability for callers to provide a pre-built client is removed.  Callers may specify the internal client
-  details by providing an instance of either `Http2SolrClient.Builder` or `HttpJdkSolrClient.Builder`. (James Dyer)
-
 Optimizations
 ---------------------
 * SOLR-17568: The CLI bin/solr export tool now contacts the appropriate nodes directly for data instead of proxying through one.
@@ -106,11 +102,6 @@ Deprecation Removals
 
 * SOLR-17540: Removed the Hadoop Auth module, and thus Kerberos authentication and other exotic options. (Eric Pugh)
 
-* SOLR-17541: Removed `CloudHttp2SolrClient.Builder#withHttpClient` in favor of `CloudHttp2SolrClient.Builder#withInternalClientBuilder`.
-  The constructor on `LBHttp2SolrClient.Builder` that took an instance of `HttpSolrClientBase` is updated to instead take an instance of
-  `HttpSolrClientBuilderBase`.  Renamed `LBHttp2SolrClient.Builder#withListenerFactory` to `LBHttp2SolrClient.Builder#withListenerFactories`
-  (James Dyer)
-
 Dependency Upgrades
 ---------------------
 (No changes)
@@ -159,10 +150,7 @@ New Features
 
 Improvements
 ---------------------
-* SOLR-17541: Deprecate `CloudHttp2SolrClient.Builder#withHttpClient` in favor of
-  `CloudHttp2SolrClient.Builder#withInternalClientBuilder`.
-   Deprecate `LBHttp2SolrClient.Builder#withListenerFactory` in favor of
-   `LBHttp2SolrClient.Builder#withListenerFactories` (James Dyer)
+(No changes)
 
 Optimizations
 ---------------------

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

@@ -197,6 +197,9 @@ 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
@@ -751,6 +754,7 @@ public void close() {
       sysPropsCacher.close();
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudManager));
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudSolrClient));
+      customThreadPool.execute(() -> IOUtils.closeQuietly(http2SolrClient));
 
       try {
         try {
@@ -846,14 +850,15 @@ public SolrCloudManager getSolrCloudManager() {
       if (cloudManager != null) {
         return cloudManager;
       }
-      var httpSolrClientBuilder =
+      http2SolrClient =
           new Http2SolrClient.Builder()
               .withHttpClient(cc.getDefaultHttpSolrClient())
               .withIdleTimeout(30000, TimeUnit.MILLISECONDS)
-              .withConnectionTimeout(15000, TimeUnit.MILLISECONDS);
+              .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
+              .build();
       cloudSolrClient =
           new CloudHttp2SolrClient.Builder(new ZkClientClusterStateProvider(zkStateReader))
-              .withInternalClientBuilder(httpSolrClientBuilder)
+              .withHttpClient(http2SolrClient)
               .build();
       cloudManager = new SolrClientCloudManager(cloudSolrClient, cc.getObjectCache());
       cloudManager.getClusterStateProvider().connect();

solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java

@@ -16,6 +16,7 @@
  */
 package org.apache.solr.core;
 
+import java.util.List;
 import java.util.concurrent.TimeUnit;
 import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.common.util.IOUtils;
@@ -36,24 +37,22 @@ final class HttpSolrClientProvider implements AutoCloseable {
 
   private final Http2SolrClient httpSolrClient;
 
-  private final Http2SolrClient.Builder httpSolrClientBuilder;
-
   private final InstrumentedHttpListenerFactory trackHttpSolrMetrics;
 
   HttpSolrClientProvider(UpdateShardHandlerConfig cfg, SolrMetricsContext parentContext) {
     trackHttpSolrMetrics = new InstrumentedHttpListenerFactory(getNameStrategy(cfg));
     initializeMetrics(parentContext);
 
-    this.httpSolrClientBuilder =
-        new Http2SolrClient.Builder().addListenerFactory(trackHttpSolrMetrics);
+    Http2SolrClient.Builder httpClientBuilder =
+        new Http2SolrClient.Builder().withListenerFactory(List.of(trackHttpSolrMetrics));
 
     if (cfg != null) {
-      httpSolrClientBuilder
+      httpClientBuilder
           .withConnectionTimeout(cfg.getDistributedConnectionTimeout(), TimeUnit.MILLISECONDS)
           .withIdleTimeout(cfg.getDistributedSocketTimeout(), TimeUnit.MILLISECONDS)
           .withMaxConnectionsPerHost(cfg.getMaxUpdateConnectionsPerHost());
     }
-    httpSolrClient = httpSolrClientBuilder.build();
+    httpSolrClient = httpClientBuilder.build();
   }
 
   private InstrumentedHttpListenerFactory.NameStrategy getNameStrategy(
@@ -77,7 +76,7 @@ Http2SolrClient getSolrClient() {
   }
 
   void setSecurityBuilder(HttpClientBuilderPlugin builder) {
-    builder.setup(httpSolrClientBuilder, httpSolrClient);
+    builder.setup(httpSolrClient);
   }
 
   @Override

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.Builder> lbClient;
+  protected LBHttp2SolrClient<Http2SolrClient> lbClient;
 
   public HttpShardHandler(HttpShardHandlerFactory httpShardHandlerFactory) {
     this.httpShardHandlerFactory = httpShardHandlerFactory;

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

@@ -83,9 +83,8 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
   protected ExecutorService commExecutor;
 
   protected volatile Http2SolrClient defaultClient;
-  protected Http2SolrClient.Builder httpSolrClientBuilder;
   protected InstrumentedHttpListenerFactory httpListenerFactory;
-  protected LBHttp2SolrClient<Http2SolrClient.Builder> loadbalancer;
+  protected LBHttp2SolrClient<Http2SolrClient> loadbalancer;
 
   int corePoolSize = 0;
   int maximumPoolSize = Integer.MAX_VALUE;
@@ -306,16 +305,16 @@ public void init(PluginInfo info) {
             sb);
     int soTimeout =
         getParameter(args, HttpClientUtil.PROP_SO_TIMEOUT, HttpClientUtil.DEFAULT_SO_TIMEOUT, sb);
-    this.httpSolrClientBuilder =
+
+    this.defaultClient =
         new Http2SolrClient.Builder()
             .withConnectionTimeout(connectionTimeout, TimeUnit.MILLISECONDS)
             .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
             .withExecutor(commExecutor)
             .withMaxConnectionsPerHost(maxConnectionsPerHost)
-            .addListenerFactory(this.httpListenerFactory);
-    this.defaultClient = httpSolrClientBuilder.build();
-
-    this.loadbalancer = new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder).build();
+            .build();
+    this.defaultClient.addListenerFactory(this.httpListenerFactory);
+    this.loadbalancer = new LBHttp2SolrClient.Builder<Http2SolrClient>(defaultClient).build();
 
     initReplicaListTransformers(getParameter(args, "replicaRouting", null, sb));
 
@@ -325,7 +324,7 @@ public void init(PluginInfo info) {
   @Override
   public void setSecurityBuilder(HttpClientBuilderPlugin clientBuilderPlugin) {
     if (clientBuilderPlugin != null) {
-      clientBuilderPlugin.setup(httpSolrClientBuilder, defaultClient);
+      clientBuilderPlugin.setup(defaultClient);
     }
   }
 

solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java

@@ -34,9 +34,4 @@ public interface HttpClientBuilderPlugin {
   public SolrHttpClientBuilder getHttpClientBuilder(SolrHttpClientBuilder builder);
 
   public default void setup(Http2SolrClient client) {}
-
-  /** TODO: Ideally, we only pass the builder here. */
-  public default void setup(Http2SolrClient.Builder builder, Http2SolrClient client) {
-    setup(client);
-  }
 }

solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java

@@ -376,11 +376,6 @@ PublicKey fetchPublicKeyFromRemote(String nodename) {
 
   @Override
   public void setup(Http2SolrClient client) {
-    setup(null, client);
-  }
-
-  @Override
-  public void setup(Http2SolrClient.Builder builder, Http2SolrClient client) {
     final HttpListenerFactory.RequestResponseListener listener =
         new HttpListenerFactory.RequestResponseListener() {
           private static final String CACHED_REQUEST_USER_KEY = "cachedRequestUser";
@@ -436,12 +431,7 @@ private Optional<String> getUserFromJettyRequest(Request request) {
                 (String) request.getAttributes().get(CACHED_REQUEST_USER_KEY));
           }
         };
-    if (client != null) {
-      client.addListenerFactory(() -> listener);
-    }
-    if (builder != null) {
-      builder.addListenerFactory(() -> listener);
-    }
+    client.addListenerFactory(() -> listener);
   }
 
   @Override

solr/core/src/test/org/apache/solr/cli/PostToolTest.java

@@ -76,7 +76,6 @@ public void testBasicRun() throws Exception {
 
     withBasicAuth(CollectionAdminRequest.createCollection(collection, "conf1", 1, 1, 0, 0))
         .processAndWait(cluster.getSolrClient(), 10);
-    waitForState("creating", collection, activeClusterShape(1, 1));
 
     File jsonDoc = File.createTempFile("temp", ".json");
 

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

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

solr/solrj/src/java/org/apache/solr/client/solrj/SolrClientFunction.java

@@ -18,11 +18,7 @@
 
 import java.io.IOException;
 
-/**
- * A lambda intended for invoking SolrClient operations
- *
- * @lucene.experimental
- */
+/** A lambda intended for invoking SolrClient operations */
 @FunctionalInterface
 public interface SolrClientFunction<C extends SolrClient, R> {
   R apply(C c) throws IOException, SolrServerException;

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

@@ -40,8 +40,9 @@
 public class CloudHttp2SolrClient extends CloudSolrClient {
 
   private final ClusterStateProvider stateProvider;
-  private final LBHttp2SolrClient<Http2SolrClient.Builder> lbClient;
+  private final LBHttp2SolrClient<Http2SolrClient> 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
@@ -53,8 +54,8 @@ public class CloudHttp2SolrClient extends CloudSolrClient {
    */
   protected CloudHttp2SolrClient(Builder builder) {
     super(builder.shardLeadersOnly, builder.parallelUpdates, builder.directUpdatesToLeadersOnly);
-    var httpSolrClientBuilder = createOrGetHttpClientBuilder(builder);
-    this.myClient = httpSolrClientBuilder.build();
+    this.clientIsInternal = builder.httpClient == null;
+    this.myClient = createOrGetHttpClientFromBuilder(builder);
     this.stateProvider = createClusterStateProvider(builder);
     this.retryExpiryTimeNano = builder.retryExpiryTimeNano;
     this.defaultCollection = builder.defaultCollection;
@@ -72,14 +73,16 @@ protected CloudHttp2SolrClient(Builder builder) {
     // locks.
     this.locks = objectList(builder.parallelCacheRefreshesLocks);
 
-    this.lbClient = new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder).build();
+    this.lbClient = new LBHttp2SolrClient.Builder<Http2SolrClient>(myClient).build();
   }
 
-  private Http2SolrClient.Builder createOrGetHttpClientBuilder(Builder builder) {
-    if (builder.internalClientBuilder != null) {
-      return builder.internalClientBuilder;
+  private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
+    if (builder.httpClient != null) {
+      return builder.httpClient;
+    } else if (builder.internalClientBuilder != null) {
+      return builder.internalClientBuilder.build();
     } else {
-      return new Http2SolrClient.Builder();
+      return new Http2SolrClient.Builder().build();
     }
   }
 
@@ -126,7 +129,7 @@ private ClusterStateProvider createHttp2ClusterStateProvider(
 
   private void closeMyClientIfNeeded() {
     try {
-      if (myClient != null) {
+      if (clientIsInternal && myClient != null) {
         myClient.close();
       }
     } catch (Exception e) {
@@ -145,7 +148,7 @@ public void close() throws IOException {
   }
 
   @Override
-  public LBHttp2SolrClient<Http2SolrClient.Builder> getLbClient() {
+  public LBHttp2SolrClient<Http2SolrClient> getLbClient() {
     return lbClient;
   }
 
@@ -168,6 +171,7 @@ 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;
@@ -400,6 +404,23 @@ 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
@@ -409,6 +430,10 @@ public Builder withCollectionCacheTtl(long timeToLive, TimeUnit unit) {
      * @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;
     }