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

solr/CHANGES.txt

@@ -34,6 +34,10 @@ 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.
@@ -102,6 +106,11 @@ 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)
@@ -150,7 +159,10 @@ New Features
 
 Improvements
 ---------------------
-(No changes)
+* SOLR-17541: Deprecate `CloudHttp2SolrClient.Builder#withHttpClient` in favor of
+  `CloudHttp2SolrClient.Builder#withInternalClientBuilder`.
+   Deprecate `LBHttp2SolrClient.Builder#withListenerFactory` in favor of
+   `LBHttp2SolrClient.Builder#withListenerFactories` (James Dyer)
 
 Optimizations
 ---------------------

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

@@ -197,9 +197,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
@@ -754,7 +751,6 @@ public void close() {
       sysPropsCacher.close();
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudManager));
       customThreadPool.execute(() -> IOUtils.closeQuietly(cloudSolrClient));
-      customThreadPool.execute(() -> IOUtils.closeQuietly(http2SolrClient));
 
       try {
         try {
@@ -850,15 +846,14 @@ public SolrCloudManager getSolrCloudManager() {
       if (cloudManager != null) {
         return cloudManager;
       }
-      http2SolrClient =
+      var httpSolrClientBuilder =
           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(httpSolrClientBuilder)
               .build();
       cloudManager = new SolrClientCloudManager(cloudSolrClient, cc.getObjectCache());
       cloudManager.getClusterStateProvider().connect();

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

@@ -16,7 +16,6 @@
  */
 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;
@@ -37,22 +36,24 @@ 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);
 
-    Http2SolrClient.Builder httpClientBuilder =
-        new Http2SolrClient.Builder().withListenerFactory(List.of(trackHttpSolrMetrics));
+    this.httpSolrClientBuilder =
+        new Http2SolrClient.Builder().addListenerFactory(trackHttpSolrMetrics);
 
     if (cfg != null) {
-      httpClientBuilder
+      httpSolrClientBuilder
           .withConnectionTimeout(cfg.getDistributedConnectionTimeout(), TimeUnit.MILLISECONDS)
           .withIdleTimeout(cfg.getDistributedSocketTimeout(), TimeUnit.MILLISECONDS)
           .withMaxConnectionsPerHost(cfg.getMaxUpdateConnectionsPerHost());
     }
-    httpSolrClient = httpClientBuilder.build();
+    httpSolrClient = httpSolrClientBuilder.build();
   }
 
   private InstrumentedHttpListenerFactory.NameStrategy getNameStrategy(
@@ -76,7 +77,7 @@ Http2SolrClient getSolrClient() {
   }
 
   void setSecurityBuilder(HttpClientBuilderPlugin builder) {
-    builder.setup(httpSolrClient);
+    builder.setup(httpSolrClientBuilder, 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> lbClient;
+  protected LBHttp2SolrClient<Http2SolrClient.Builder> lbClient;
 
   public HttpShardHandler(HttpShardHandlerFactory httpShardHandlerFactory) {
     this.httpShardHandlerFactory = httpShardHandlerFactory;

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

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

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

@@ -34,4 +34,9 @@ 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,6 +376,11 @@ 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";
@@ -431,7 +436,12 @@ private Optional<String> getUserFromJettyRequest(Request request) {
                 (String) request.getAttributes().get(CACHED_REQUEST_USER_KEY));
           }
         };
-    client.addListenerFactory(() -> listener);
+    if (client != null) {
+      client.addListenerFactory(() -> listener);
+    }
+    if (builder != null) {
+      builder.addListenerFactory(() -> listener);
+    }
   }
 
   @Override

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

@@ -76,6 +76,7 @@ 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,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/SolrClientFunction.java

@@ -18,7 +18,11 @@
 
 import java.io.IOException;
 
-/** A lambda intended for invoking SolrClient operations */
+/**
+ * A lambda intended for invoking SolrClient operations
+ *
+ * @lucene.experimental
+ */
 @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,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 httpSolrClientBuilder = createOrGetHttpClientBuilder(builder);
+    this.myClient = httpSolrClientBuilder.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<>(httpSolrClientBuilder).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;
     }