diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index a844220f2fc..a135ecaa539 100644 --- a/solr/CHANGES.txt +++ b/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 --------------------- diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkController.java b/solr/core/src/java/org/apache/solr/cloud/ZkController.java index 7dd30a14d24..5a890121a43 100644 --- a/solr/core/src/java/org/apache/solr/cloud/ZkController.java +++ b/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(); diff --git a/solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java b/solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java index 2bf25a896f6..e9631b26d1f 100644 --- a/solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java +++ b/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 diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java index 7592eed86fc..320a5fe70d7 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java +++ b/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> shardToURLs; - protected LBHttp2SolrClient lbClient; + protected LBHttp2SolrClient lbClient; public HttpShardHandler(HttpShardHandlerFactory httpShardHandlerFactory) { this.httpShardHandlerFactory = httpShardHandlerFactory; diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java index ac7dc0cf8e0..1437dee63ea 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java +++ b/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 loadbalancer; + protected LBHttp2SolrClient 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(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); } } diff --git a/solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java b/solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java index b43d5f22190..206fb3d0886 100644 --- a/solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java +++ b/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); + } } diff --git a/solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java b/solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java index b1f6e6b6eed..93ecdcc9d68 100644 --- a/solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java +++ b/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 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 diff --git a/solr/core/src/test/org/apache/solr/cli/PostToolTest.java b/solr/core/src/test/org/apache/solr/cli/PostToolTest.java index 758ae9ccd51..9eb3e783a2d 100644 --- a/solr/core/src/test/org/apache/solr/cli/PostToolTest.java +++ b/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"); diff --git a/solr/core/src/test/org/apache/solr/cloud/OverseerTest.java b/solr/core/src/test/org/apache/solr/cloud/OverseerTest.java index bad8d58d021..e611902898f 100644 --- a/solr/core/src/test/org/apache/solr/cloud/OverseerTest.java +++ b/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; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClientFunction.java b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClientFunction.java index 0adb49471dc..68246001a40 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClientFunction.java +++ b/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 { R apply(C c) throws IOException, SolrServerException; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java index ade1ebe433f..18aa318f8ba 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java +++ b/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 lbClient; + private final LBHttp2SolrClient 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(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 getLbClient() { + public LBHttp2SolrClient getLbClient() { return lbClient; } @@ -171,7 +168,6 @@ public static class Builder { protected Collection zkHosts = new ArrayList<>(); protected List 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. - * - *

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; } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java index fb2eb1a123f..fddc4e2daa6 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java @@ -140,9 +140,7 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) { this.httpClient = createHttpClient(builder); this.closeClient = true; } - if (builder.listenerFactory != null) { - this.listenerFactory.addAll(builder.listenerFactory); - } + this.listenerFactory.addAll(builder.listenerFactories); updateDefaultMimeTypeForParser(); this.httpClient.setFollowRedirects(Boolean.TRUE.equals(builder.followRedirects)); @@ -571,6 +569,7 @@ public final R requestWithBaseUrl( * @param clientFunction a Function that consumes a Http2SolrClient and returns an arbitrary value * @return the value returned after invoking 'clientFunction' * @param the type returned by the provided function (and by this method) + * @lucene.experimental */ public R requestWithBaseUrl( String baseUrl, SolrClientFunction clientFunction) @@ -906,7 +905,7 @@ public static class Builder protected Long keyStoreReloadIntervalSecs; - private List listenerFactory; + private List listenerFactories = new ArrayList<>(0); public Builder() { super(); @@ -932,8 +931,27 @@ public Builder(String baseSolrUrl) { this.baseSolrUrl = baseSolrUrl; } - public Http2SolrClient.Builder withListenerFactory(List listenerFactory) { - this.listenerFactory = listenerFactory; + /** + * specify a listener factory, which will be appended to any existing values. + * + * @param listenerFactory a HttpListenerFactory + * @return This Builder + */ + public Http2SolrClient.Builder addListenerFactory(HttpListenerFactory listenerFactory) { + this.listenerFactories.add(listenerFactory); + return this; + } + + /** + * Specify listener factories, which will replace any existing values. + * + * @param listenerFactories a list of HttpListenerFactory instances + * @return This Builder + */ + public Http2SolrClient.Builder withListenerFactories( + List listenerFactories) { + this.listenerFactories.clear(); + this.listenerFactories.addAll(listenerFactories); return this; } @@ -1109,9 +1127,9 @@ public Builder withHttpClient(Http2SolrClient http2SolrClient) { if (this.urlParamNames == null) { this.urlParamNames = http2SolrClient.urlParamNames; } - if (this.listenerFactory == null) { - this.listenerFactory = new ArrayList(); - http2SolrClient.listenerFactory.forEach(this.listenerFactory::add); + if (this.listenerFactories.isEmpty()) { + this.listenerFactories.clear(); + http2SolrClient.listenerFactory.forEach(this.listenerFactories::add); } if (this.executor == null) { this.executor = http2SolrClient.executor; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index 1127b3fd1a1..c26d13606b6 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/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> 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> requestAsync( } } - protected NamedList requestWithBaseUrl( - String baseUrl, SolrRequest solrRequest, String collection) + @Override + public NamedList request(SolrRequest solrRequest, String collection) throws SolrServerException, IOException { - PreparedRequest pReq = prepareRequest(solrRequest, collection, baseUrl); + PreparedRequest pReq = prepareRequest(solrRequest, collection); HttpResponse response = null; try { response = httpClient.send(pReq.reqb.build(), HttpResponse.BodyHandlers.ofInputStream()); @@ -192,25 +192,13 @@ protected NamedList requestWithBaseUrl( } } - @Override - public NamedList 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(); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java index 2c926a26261..4c0d46b13db 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java @@ -23,22 +23,25 @@ import java.net.SocketException; import java.net.SocketTimeoutException; import java.util.Arrays; +import java.util.Collections; +import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import org.apache.solr.client.solrj.ResponseParser; -import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.IsUpdateRequest; import org.apache.solr.client.solrj.request.RequestWriter; import org.apache.solr.common.SolrException; +import org.apache.solr.common.util.IOUtils; import org.apache.solr.common.util.NamedList; import org.slf4j.MDC; /** - * This "LoadBalanced Http Solr Client" is a load balancing wrapper around a Http Solr Client. This - * is useful when you have multiple Solr endpoints and requests need to be Load Balanced among them. + * This "LoadBalanced Http Solr Client" is a load balancer for multiple Http Solr Clients. This is + * useful when you have multiple Solr endpoints and requests need to be Load Balanced among them. * *

Do NOT use this class for indexing in leader/follower scenarios since documents must be * sent to the correct leader; no inter-node routing is done. @@ -56,7 +59,7 @@ *

* *
- * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClient,
+ * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClientBuilder,
  *         new LBSolrClient.Endpoint("http://host1:8080/solr"), new LBSolrClient.Endpoint("http://host2:8080/solr"))
  *     .build();
  * 
@@ -69,7 +72,7 @@ *
* *
- * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClient,
+ * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClientBuilder,
  *         new LBSolrClient.Endpoint("http://host1:8080/solr", "coreA"),
  *         new LBSolrClient.Endpoint("http://host2:8080/solr", "coreB"))
  *     .build();
@@ -94,35 +97,63 @@
  *
  * @since solr 8.0
  */
-public class LBHttp2SolrClient extends LBSolrClient {
+public class LBHttp2SolrClient> extends LBSolrClient {
 
-  protected final C solrClient;
+  private final Map urlToClient;
+  private final Set urlParamNames;
+
+  // must synchronize on this when building
+  private final HttpSolrClientBuilderBase solrClientBuilder;
 
-  @SuppressWarnings("unchecked")
   private LBHttp2SolrClient(Builder builder) {
     super(Arrays.asList(builder.solrEndpoints));
-    this.solrClient = (C) builder.solrClient;
+    this.solrClientBuilder = builder.solrClientBuilder;
+
     this.aliveCheckIntervalMillis = builder.aliveCheckIntervalMillis;
     this.defaultCollection = builder.defaultCollection;
+
+    if (builder.solrClientBuilder.urlParamNames == null) {
+      this.urlParamNames = Collections.emptySet();
+    } else {
+      this.urlParamNames = Set.copyOf(builder.solrClientBuilder.urlParamNames);
+    }
+
+    this.urlToClient = new ConcurrentHashMap<>();
+    for (LBSolrClient.Endpoint endpoint : builder.solrEndpoints) {
+      getClient(endpoint);
+    }
   }
 
   @Override
-  protected SolrClient getClient(Endpoint endpoint) {
-    return solrClient;
+  protected HttpSolrClientBase getClient(final Endpoint endpoint) {
+    return urlToClient.computeIfAbsent(
+        endpoint.getBaseUrl(),
+        url -> {
+          synchronized (solrClientBuilder) {
+            solrClientBuilder.baseSolrUrl = url;
+            return solrClientBuilder.build();
+          }
+        });
   }
 
   @Override
   public ResponseParser getParser() {
-    return solrClient.getParser();
+    return urlToClient.isEmpty() ? null : urlToClient.values().iterator().next().getParser();
   }
 
   @Override
   public RequestWriter getRequestWriter() {
-    return solrClient.getRequestWriter();
+    return urlToClient.isEmpty() ? null : urlToClient.values().iterator().next().getRequestWriter();
   }
 
   public Set getUrlParamNames() {
-    return solrClient.getUrlParamNames();
+    return urlParamNames;
+  }
+
+  @Override
+  public void close() {
+    urlToClient.values().forEach(IOUtils::closeQuietly);
+    super.close();
   }
 
   /**
@@ -210,23 +241,18 @@ private CompletableFuture> doAsyncRequest(
       RetryListener listener) {
     String baseUrl = endpoint.toString();
     rsp.server = baseUrl;
-    final var client = (Http2SolrClient) getClient(endpoint);
-    try {
-      CompletableFuture> future =
-          client.requestWithBaseUrl(baseUrl, (c) -> c.requestAsync(req.getRequest()));
-      future.whenComplete(
-          (result, throwable) -> {
-            if (!future.isCompletedExceptionally()) {
-              onSuccessfulRequest(result, endpoint, rsp, isZombie, listener);
-            } else if (!future.isCancelled()) {
-              onFailedRequest(throwable, endpoint, isNonRetryable, isZombie, listener);
-            }
-          });
-      return future;
-    } catch (SolrServerException | IOException e) {
-      // Unreachable, since 'requestWithBaseUrl' above is running the request asynchronously
-      throw new RuntimeException(e);
-    }
+    final var client = getClient(endpoint);
+    CompletableFuture> future =
+        client.requestAsync(req.getRequest(), endpoint.getCore());
+    future.whenComplete(
+        (result, throwable) -> {
+          if (!future.isCompletedExceptionally()) {
+            onSuccessfulRequest(result, endpoint, rsp, isZombie, listener);
+          } else if (!future.isCancelled()) {
+            onFailedRequest(throwable, endpoint, isNonRetryable, isZombie, listener);
+          }
+        });
+    return future;
   }
 
   private void onSuccessfulRequest(
@@ -290,16 +316,28 @@ private void onFailedRequest(
     }
   }
 
-  public static class Builder {
+  public static class Builder> {
+
+    private final B solrClientBuilder;
 
-    private final C solrClient;
     private final LBSolrClient.Endpoint[] solrEndpoints;
     private long aliveCheckIntervalMillis =
         TimeUnit.MILLISECONDS.convert(60, TimeUnit.SECONDS); // 1 minute between checks
     protected String defaultCollection;
 
-    public Builder(C solrClient, Endpoint... endpoints) {
-      this.solrClient = solrClient;
+    /**
+     * Use this Builder to configure an LBHttp2SolrClient. The passed-in Solr Client Builder will be
+     * used to generate an internal client per Endpoint.
+     *
+     * 

Implementation Note: LBHttp2SolrClient will modify the passed-in Builder's {@link + * HttpSolrClientBuilderBase#baseSolrUrl} whenever it needs to generate a new Http Solr Client. + * + * @param solrClientBuilder A Builder like {@link Http2SolrClient.Builder} used to generate the + * internal clients + * @param endpoints the initial Solr Endpoints to load balance + */ + public Builder(B solrClientBuilder, Endpoint... endpoints) { + this.solrClientBuilder = solrClientBuilder; this.solrEndpoints = endpoints; } @@ -309,7 +347,7 @@ public Builder(C solrClient, Endpoint... endpoints) { * * @param aliveCheckInterval how often to ping for aliveness */ - public Builder setAliveCheckInterval(int aliveCheckInterval, TimeUnit unit) { + public Builder setAliveCheckInterval(int aliveCheckInterval, TimeUnit unit) { if (aliveCheckInterval <= 0) { throw new IllegalArgumentException( "Alive check interval must be " + "positive, specified value = " + aliveCheckInterval); @@ -319,13 +357,13 @@ public Builder setAliveCheckInterval(int aliveCheckInterval, TimeUnit unit) { } /** Sets a default for core or collection based requests. */ - public Builder withDefaultCollection(String defaultCoreOrCollection) { + public Builder withDefaultCollection(String defaultCoreOrCollection) { this.defaultCollection = defaultCoreOrCollection; return this; } - public LBHttp2SolrClient build() { - return new LBHttp2SolrClient(this); + public LBHttp2SolrClient build() { + return new LBHttp2SolrClient(this); } } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 64201b03c13..31c75662b48 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -497,25 +497,7 @@ private static boolean isTimeExceeded(long timeAllowedNano, long timeOutTime) { private NamedList doRequest(Endpoint endpoint, SolrRequest solrRequest) throws SolrServerException, IOException { final var solrClient = getClient(endpoint); - return doRequest(solrClient, endpoint.getBaseUrl(), endpoint.getCore(), solrRequest); - } - - // TODO SOLR-17541 should remove the need for the special-casing below; remove as a part of that - // ticket. - private NamedList doRequest( - SolrClient solrClient, String baseUrl, String collection, SolrRequest solrRequest) - throws SolrServerException, IOException { - // Some implementations of LBSolrClient.getClient(...) return a Http2SolrClient that may not be - // pointed at the desired URL (or any URL for that matter). We special case that here to ensure - // the appropriate URL is provided. - if (solrClient instanceof Http2SolrClient httpSolrClient) { - return httpSolrClient.requestWithBaseUrl(baseUrl, (c) -> c.request(solrRequest, collection)); - } else if (solrClient instanceof HttpJdkSolrClient) { - return ((HttpJdkSolrClient) solrClient).requestWithBaseUrl(baseUrl, solrRequest, collection); - } - - // Assume provided client already uses 'baseUrl' - return solrClient.request(solrRequest, collection); + return solrClient.request(solrRequest, endpoint.getCore()); } protected Exception doRequest( @@ -625,12 +607,7 @@ private void checkAZombieServer(EndpointWrapper zombieServer) { // First the one on the endpoint, then the default collection final String effectiveCollection = Objects.requireNonNullElse(zombieEndpoint.getCore(), getDefaultCollection()); - final var responseRaw = - doRequest( - getClient(zombieEndpoint), - zombieEndpoint.getBaseUrl(), - effectiveCollection, - queryRequest); + final var responseRaw = getClient(zombieEndpoint).request(queryRequest, effectiveCollection); QueryResponse resp = new QueryResponse(); resp.setResponse(responseRaw); @@ -734,7 +711,7 @@ public NamedList request( // Choose the endpoint's core/collection over any specified by the user final var effectiveCollection = endpoint.getCore() == null ? collection : endpoint.getCore(); - return doRequest(getClient(endpoint), endpoint.getBaseUrl(), effectiveCollection, request); + return getClient(endpoint).request(request, effectiveCollection); } catch (SolrException e) { // Server is alive but the request was malformed or invalid throw e; @@ -775,8 +752,7 @@ public NamedList request( ++numServersTried; final String effectiveCollection = endpoint.getCore() == null ? collection : endpoint.getCore(); - NamedList rsp = - doRequest(getClient(endpoint), endpoint.getBaseUrl(), effectiveCollection, request); + NamedList rsp = getClient(endpoint).request(request, effectiveCollection); // remove from zombie list *before* adding to the alive list to avoid a race that could lose // a server zombieServers.remove(endpoint.getUrl()); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java index 0846dfefc6c..46b6883f755 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java @@ -120,26 +120,6 @@ public void testIsDirectUpdatesToLeadersOnlyDefault() throws IOException { } } - @Test - public void testExternalClientAndInternalBuilderTogether() { - expectThrows( - IllegalStateException.class, - () -> - new CloudHttp2SolrClient.Builder( - Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT)) - .withHttpClient(mock(Http2SolrClient.class)) - .withInternalClientBuilder(mock(Http2SolrClient.Builder.class)) - .build()); - expectThrows( - IllegalStateException.class, - () -> - new CloudHttp2SolrClient.Builder( - Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT)) - .withInternalClientBuilder(mock(Http2SolrClient.Builder.class)) - .withHttpClient(mock(Http2SolrClient.class)) - .build()); - } - @Test public void testProvideInternalBuilder() throws IOException { Http2SolrClient http2Client = mock(Http2SolrClient.class); @@ -159,20 +139,6 @@ public void testProvideInternalBuilder() throws IOException { verify(http2Client, times(1)).close(); } - @Test - public void testProvideExternalClient() throws IOException { - Http2SolrClient http2Client = mock(Http2SolrClient.class); - CloudHttp2SolrClient.Builder clientBuilder = - new CloudHttp2SolrClient.Builder( - Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT)) - .withHttpClient(http2Client); - try (CloudHttp2SolrClient client = clientBuilder.build()) { - assertEquals(http2Client, client.getHttpClient()); - } - // it's external, should be NOT closed when closing CloudSolrClient - verify(http2Client, never()).close(); - } - @Test public void testDefaultCollectionPassedFromBuilderToClient() throws IOException { try (CloudHttp2SolrClient createdClient = @@ -195,29 +161,19 @@ public void testDefaultCollectionPassedFromBuilderToClient() throws IOException public void testHttpClientPreservedInHttp2ClusterStateProvider() throws IOException { List solrUrls = List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString()); - // No httpClient - No internalClientBuilder - testHttpClientConsistency(solrUrls, null, null); - - // httpClient - No internalClientBuilder - try (Http2SolrClient httpClient = new Http2SolrClient.Builder().build()) { - testHttpClientConsistency(solrUrls, httpClient, null); - } + // without internalClientBuilder + testHttpClientConsistency(solrUrls, null); - // No httpClient - internalClientBuilder + // with internalClientBuilder Http2SolrClient.Builder internalClientBuilder = new Http2SolrClient.Builder(); - testHttpClientConsistency(solrUrls, null, internalClientBuilder); + testHttpClientConsistency(solrUrls, internalClientBuilder); } private void testHttpClientConsistency( - List solrUrls, - Http2SolrClient httpClient, - Http2SolrClient.Builder internalClientBuilder) - throws IOException { + List solrUrls, Http2SolrClient.Builder internalClientBuilder) throws IOException { CloudHttp2SolrClient.Builder clientBuilder = new CloudHttp2SolrClient.Builder(solrUrls); - if (httpClient != null) { - clientBuilder.withHttpClient(httpClient); - } else if (internalClientBuilder != null) { + if (internalClientBuilder != null) { clientBuilder.withInternalClientBuilder(internalClientBuilder); } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java index b3980ad44bc..1dbfc0e998b 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java @@ -34,7 +34,6 @@ import org.apache.solr.client.solrj.SolrQuery; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; -import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.request.RequestWriter; import org.apache.solr.client.solrj.response.SolrPingResponse; import org.apache.solr.common.params.CommonParams; @@ -154,25 +153,6 @@ protected void testQuerySetup(SolrRequest.METHOD method, ResponseParser rp) thro } } - @Test - public void testRequestWithBaseUrl() throws Exception { - DebugServlet.clear(); - DebugServlet.addResponseHeader("Content-Type", "application/octet-stream"); - DebugServlet.responseBodyByQueryFragment.put("", javabinResponse()); - String someOtherUrl = getBaseUrl() + "/some/other/base/url"; - String intendedUrl = getBaseUrl() + DEBUG_SERVLET_PATH; - SolrQuery q = new SolrQuery("foo"); - q.setParam("a", MUST_ENCODE); - - HttpJdkSolrClient.Builder b = - builder(someOtherUrl).withResponseParser(new BinaryResponseParser()); - try (HttpJdkSolrClient client = b.build()) { - client.requestWithBaseUrl(intendedUrl, new QueryRequest(q, SolrRequest.METHOD.GET), null); - assertEquals( - client.getParser().getVersion(), DebugServlet.parameters.get(CommonParams.VERSION)[0]); - } - } - @Test public void testGetById() throws Exception { DebugServlet.clear(); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java index 61504a052b8..9c2f79ff0a5 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java @@ -18,7 +18,6 @@ import java.io.File; import java.io.IOException; -import java.io.UncheckedIOException; import java.lang.invoke.MethodHandles; import java.nio.file.Files; import java.nio.file.Path; @@ -118,30 +117,28 @@ public void tearDown() throws Exception { private LBClientHolder client(LBSolrClient.Endpoint... baseSolrEndpoints) { if (random().nextBoolean()) { - var delegateClient = + var delegateClientBuilder = new Http2SolrClient.Builder() .withConnectionTimeout(1000, TimeUnit.MILLISECONDS) - .withIdleTimeout(2000, TimeUnit.MILLISECONDS) - .build(); + .withIdleTimeout(2000, TimeUnit.MILLISECONDS); var lbClient = - new LBHttp2SolrClient.Builder<>(delegateClient, baseSolrEndpoints) + new LBHttp2SolrClient.Builder<>(delegateClientBuilder, baseSolrEndpoints) .withDefaultCollection(solr[0].getDefaultCollection()) .setAliveCheckInterval(500, TimeUnit.MILLISECONDS) .build(); - return new LBClientHolder(lbClient, delegateClient); + return new LBClientHolder(lbClient, delegateClientBuilder); } else { - var delegateClient = + var delegateClientBuilder = new HttpJdkSolrClient.Builder() .withConnectionTimeout(1000, TimeUnit.MILLISECONDS) .withIdleTimeout(2000, TimeUnit.MILLISECONDS) - .withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT) - .build(); + .withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT); var lbClient = - new LBHttp2SolrClient.Builder<>(delegateClient, baseSolrEndpoints) + new LBHttp2SolrClient.Builder<>(delegateClientBuilder, baseSolrEndpoints) .withDefaultCollection(solr[0].getDefaultCollection()) .setAliveCheckInterval(500, TimeUnit.MILLISECONDS) .build(); - return new LBClientHolder(lbClient, delegateClient); + return new LBClientHolder(lbClient, delegateClientBuilder); } } @@ -317,9 +314,9 @@ public void startJetty() throws Exception { private static class LBClientHolder implements AutoCloseable { final LBHttp2SolrClient lbClient; - final HttpSolrClientBase delegate; + final HttpSolrClientBuilderBase delegate; - LBClientHolder(LBHttp2SolrClient lbClient, HttpSolrClientBase delegate) { + LBClientHolder(LBHttp2SolrClient lbClient, HttpSolrClientBuilderBase delegate) { this.lbClient = lbClient; this.delegate = delegate; } @@ -327,11 +324,6 @@ private static class LBClientHolder implements AutoCloseable { @Override public void close() { lbClient.close(); - try { - delegate.close(); - } catch (IOException ioe) { - throw new UncheckedIOException(ioe); - } } } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java index 9d2019309b0..30cee0804b5 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java @@ -18,6 +18,8 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -28,7 +30,6 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import org.apache.solr.SolrTestCase; -import org.apache.solr.client.solrj.SolrClientFunction; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.QueryRequest; @@ -51,11 +52,12 @@ public void testLBHttp2SolrClientWithTheseParamNamesInTheUrl() { Set urlParamNames = new HashSet<>(2); urlParamNames.add("param1"); - try (Http2SolrClient http2SolrClient = - new Http2SolrClient.Builder(url).withTheseParamNamesInTheUrl(urlParamNames).build(); - LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(http2SolrClient, new LBSolrClient.Endpoint(url)) - .build()) { + var httpSolrClientBuilder = + new Http2SolrClient.Builder(url).withTheseParamNamesInTheUrl(urlParamNames); + var endpoint = new LBSolrClient.Endpoint(url); + try (var testClient = + new LBHttp2SolrClient.Builder(httpSolrClientBuilder, endpoint) + .build()) { assertArrayEquals( "Wrong urlParamNames found in lb client.", @@ -64,7 +66,7 @@ public void testLBHttp2SolrClientWithTheseParamNamesInTheUrl() { assertArrayEquals( "Wrong urlParamNames found in base client.", urlParamNames.toArray(), - http2SolrClient.getUrlParamNames().toArray()); + testClient.getClient(endpoint).getUrlParamNames().toArray()); } } @@ -74,12 +76,11 @@ public void testSynchronous() throws Exception { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - Http2SolrClient.Builder b = - new Http2SolrClient.Builder("http://base.url").withConnectionTimeout(10, TimeUnit.SECONDS); - ; - try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b); - LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) { + var httpSolrClientBuilder = + new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); + + try (LBHttp2SolrClient testClient = + new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { String lastEndpoint = null; for (int i = 0; i < 10; i++) { @@ -103,15 +104,14 @@ public void testSynchronousWithFalures() throws Exception { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - Http2SolrClient.Builder b = - new Http2SolrClient.Builder("http://base.url").withConnectionTimeout(10, TimeUnit.SECONDS); - ; - try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b); - LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) { + var httpSolrClientBuilder = + new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); + + try (LBHttp2SolrClient testClient = + new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { - client.basePathToFail = ep1.getBaseUrl(); - String basePathToSucceed = ep2.getBaseUrl(); + setEndpointToFail(testClient, ep1); + setEndpointToSucceed(testClient, ep2); String qValue = "First time"; for (int i = 0; i < 5; i++) { @@ -121,13 +121,13 @@ public void testSynchronousWithFalures() throws Exception { LBSolrClient.Rsp response = testClient.request(req); assertEquals( "The healthy node 'endpoint two' should have served the request: " + i, - basePathToSucceed, + ep2.getBaseUrl(), response.server); checkSynchonousResponseContent(response, qValue); } - client.basePathToFail = ep2.getBaseUrl(); - basePathToSucceed = ep1.getBaseUrl(); + setEndpointToFail(testClient, ep2); + setEndpointToSucceed(testClient, ep1); qValue = "Second time"; for (int i = 0; i < 5; i++) { @@ -137,21 +137,13 @@ public void testSynchronousWithFalures() throws Exception { LBSolrClient.Rsp response = testClient.request(req); assertEquals( "The healthy node 'endpoint one' should have served the request: " + i, - basePathToSucceed, + ep1.getBaseUrl(), response.server); checkSynchonousResponseContent(response, qValue); } } } - private void checkSynchonousResponseContent(LBSolrClient.Rsp response, String qValue) { - assertEquals("There should be one element in the respnse.", 1, response.getResponse().size()); - assertEquals( - "The response key 'response' should echo the query.", - qValue, - response.getResponse().get("response")); - } - @Test public void testAsyncWithFailures() { @@ -162,28 +154,35 @@ public void testAsyncWithFailures() { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - Http2SolrClient.Builder b = - new Http2SolrClient.Builder("http://base.url").withConnectionTimeout(10, TimeUnit.SECONDS); - ; - try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b); - LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) { + var httpSolrClientBuilder = + new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); + + try (LBHttp2SolrClient testClient = + new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { for (int j = 0; j < 2; j++) { // first time Endpoint One will return error code 500. // second time Endpoint One will be healthy - String basePathToSucceed; + LBSolrClient.Endpoint endpointToSucceed; + LBSolrClient.Endpoint endpointToFail; if (j == 0) { - client.basePathToFail = ep1.getBaseUrl(); - basePathToSucceed = ep2.getBaseUrl(); + setEndpointToFail(testClient, ep1); + setEndpointToSucceed(testClient, ep2); + endpointToSucceed = ep2; + endpointToFail = ep1; } else { - client.basePathToFail = ep2.getBaseUrl(); - basePathToSucceed = ep1.getBaseUrl(); + setEndpointToFail(testClient, ep2); + setEndpointToSucceed(testClient, ep1); + endpointToSucceed = ep1; + endpointToFail = ep2; } + List successEndpointLastBasePaths = + basePathsForEndpoint(testClient, endpointToSucceed); + List failEndpointLastBasePaths = basePathsForEndpoint(testClient, endpointToFail); for (int i = 0; i < 10; i++) { - // i: we'll try 10 times to see if it behaves the same every time. + // i: we'll try 10 times. It should behave the same with iter 2-10. . QueryRequest queryRequest = new QueryRequest(new MapSolrParams(Map.of("q", "" + i))); LBSolrClient.Req req = new LBSolrClient.Req(queryRequest, endpointList); @@ -196,26 +195,26 @@ public void testAsyncWithFailures() { } catch (TimeoutException | ExecutionException e) { fail(iterMessage + " Response ended in failure: " + e); } + if (i == 0) { - // When j=0, "endpoint one" fails. - // The first time around (i) it tries the first, then the second. - // - // With j=0 and i>0, it only tries "endpoint two". + // When i=0, it must try both endpoints to find success: // - // When j=1 and i=0, "endpoint two" starts failing. - // So it tries both it and "endpoint one" - // - // With j=1 and i>0, it only tries "endpoint one". - assertEquals(iterMessage, 2, client.lastBasePaths.size()); - - String failedBasePath = client.lastBasePaths.remove(0); - assertEquals(iterMessage, client.basePathToFail, failedBasePath); + // with j=0, endpoint one is tried first because it + // is first one the list, but it fails. + // with j=1, endpoint two is tried first because + // it is the only known healthy node, but + // now it is failing. + assertEquals(iterMessage, 1, successEndpointLastBasePaths.size()); + assertEquals(iterMessage, 1, failEndpointLastBasePaths.size()); } else { - // The first endpoint does not give the exception, it doesn't retry. - assertEquals(iterMessage, 1, client.lastBasePaths.size()); + // With i>0, + // With j=0 and i>0, it only tries "endpoint two". + // With j=1 and i>0, it only tries "endpoint one". + assertEquals(iterMessage, 1, successEndpointLastBasePaths.size()); + assertTrue(iterMessage, failEndpointLastBasePaths.isEmpty()); } - String successBasePath = client.lastBasePaths.remove(0); - assertEquals(iterMessage, basePathToSucceed, successBasePath); + successEndpointLastBasePaths.clear(); + failEndpointLastBasePaths.clear(); } } } @@ -227,11 +226,11 @@ public void testAsync() { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - Http2SolrClient.Builder b = - new Http2SolrClient.Builder("http://base.url").withConnectionTimeout(10, TimeUnit.SECONDS); - try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b); - LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) { + var httpSolrClientBuilder = + new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); + + try (LBHttp2SolrClient testClient = + new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { int limit = 10; // For simplicity use an even limit List> responses = new ArrayList<>(); @@ -243,23 +242,17 @@ public void testAsync() { } QueryRequest[] queryRequests = new QueryRequest[limit]; - int numEndpointOne = 0; - int numEndpointTwo = 0; + List> lastSolrRequests = lastSolrRequests(testClient, ep1, ep2); + assertEquals(limit, lastSolrRequests.size()); + for (int i = 0; i < limit; i++) { - SolrRequest lastSolrReq = client.lastSolrRequests.get(i); + SolrRequest lastSolrReq = lastSolrRequests.get(i); assertTrue(lastSolrReq instanceof QueryRequest); QueryRequest lastQueryReq = (QueryRequest) lastSolrReq; int index = Integer.parseInt(lastQueryReq.getParams().get("q")); assertNull("Found same request twice: " + index, queryRequests[index]); queryRequests[index] = lastQueryReq; - final String lastBasePath = client.lastBasePaths.get(i); - if (lastBasePath.equals(ep1.toString())) { - numEndpointOne++; - } else if (lastBasePath.equals(ep2.toString())) { - numEndpointTwo++; - } - LBSolrClient.Rsp lastRsp = null; try { lastRsp = responses.get(index).get(); @@ -278,15 +271,55 @@ public void testAsync() { // It is the user's responsibility to shuffle the endpoints when using // async. LB Http Solr Client will always try the passed-in endpoints // in order. In this case, endpoint 1 gets all the requests! - assertEquals(limit, numEndpointOne); - assertEquals(0, numEndpointTwo); + List ep1BasePaths = basePathsForEndpoint(testClient, ep1); + List ep2BasePaths = basePathsForEndpoint(testClient, ep2); + assertEquals(limit, basePathsForEndpoint(testClient, ep1).size()); + assertEquals(0, basePathsForEndpoint(testClient, ep2).size()); + } + } + + private void checkSynchonousResponseContent(LBSolrClient.Rsp response, String qValue) { + assertEquals("There should be one element in the response.", 1, response.getResponse().size()); + assertEquals( + "The response key 'response' should echo the query.", + qValue, + response.getResponse().get("response")); + } + + private void setEndpointToFail( + LBHttp2SolrClient testClient, LBSolrClient.Endpoint ep) { + ((MockHttpSolrClient) testClient.getClient(ep)).allRequestsShallFail = true; + } - assertEquals(limit, client.lastSolrRequests.size()); - assertEquals(limit, client.lastCollections.size()); + private void setEndpointToSucceed( + LBHttp2SolrClient testClient, LBSolrClient.Endpoint ep) { + ((MockHttpSolrClient) testClient.getClient(ep)).allRequestsShallFail = false; + } + + private List basePathsForEndpoint( + LBHttp2SolrClient testClient, LBSolrClient.Endpoint ep) { + return ((MockHttpSolrClient) testClient.getClient(ep)).lastBasePaths; + } + + private List> lastSolrRequests( + LBHttp2SolrClient testClient, LBSolrClient.Endpoint... endpoints) { + return Arrays.stream(endpoints) + .map(testClient::getClient) + .map(MockHttpSolrClient.class::cast) + .flatMap(c -> c.lastSolrRequests.stream()) + .toList(); + } + + public static class MockHttpSolrClientBuilder + extends HttpSolrClientBuilderBase { + + @Override + public MockHttpSolrClient build() { + return new MockHttpSolrClient(baseSolrUrl, this); } } - public static class MockHttpSolrClient extends Http2SolrClient { + public static class MockHttpSolrClient extends HttpSolrClientBase { public List> lastSolrRequests = new ArrayList<>(); @@ -294,15 +327,13 @@ public static class MockHttpSolrClient extends Http2SolrClient { public List lastCollections = new ArrayList<>(); - public String basePathToFail = null; + public boolean allRequestsShallFail; public String tmpBaseUrl = null; - protected MockHttpSolrClient(String serverBaseUrl, Builder builder) { + public boolean closeCalled; - // TODO: Consider creating an interface for Http*SolrClient - // so mocks can Implement, not Extend, and not actually need to - // build an (unused) client + protected MockHttpSolrClient(String serverBaseUrl, MockHttpSolrClientBuilder builder) { super(serverBaseUrl, builder); } @@ -312,25 +343,12 @@ public NamedList request(final SolrRequest request, String collection lastSolrRequests.add(request); lastBasePaths.add(tmpBaseUrl); lastCollections.add(collection); - if (tmpBaseUrl.equals(basePathToFail)) { + if (allRequestsShallFail) { throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "We should retry this."); } return generateResponse(request); } - @Override - public R requestWithBaseUrl( - String baseUrl, SolrClientFunction clientFunction) - throws SolrServerException, IOException { - // This use of 'tmpBaseUrl' is NOT thread safe, but that's fine for our purposes here. - try { - tmpBaseUrl = baseUrl; - return clientFunction.apply(this); - } finally { - tmpBaseUrl = null; - } - } - @Override public CompletableFuture> requestAsync( final SolrRequest solrRequest, String collection) { @@ -338,7 +356,7 @@ public CompletableFuture> requestAsync( lastSolrRequests.add(solrRequest); lastBasePaths.add(tmpBaseUrl); lastCollections.add(collection); - if (tmpBaseUrl != null && tmpBaseUrl.equals(basePathToFail)) { + if (allRequestsShallFail) { cf.completeExceptionally( new SolrException(SolrException.ErrorCode.SERVER_ERROR, "We should retry this.")); } else { @@ -351,5 +369,32 @@ private NamedList generateResponse(SolrRequest solrRequest) { String id = solrRequest.getParams().get("q"); return new NamedList<>(Collections.singletonMap("response", id)); } + + @Override + public void close() throws IOException { + closeCalled = true; + } + + @Override + protected boolean isFollowRedirects() { + return false; + } + + @Override + protected boolean processorAcceptsMimeType( + Collection processorSupportedContentTypes, String mimeType) { + return false; + } + + @Override + protected String allProcessorSupportedContentTypesCommaDelimited( + Collection processorSupportedContentTypes) { + return null; + } + + @Override + protected void updateDefaultMimeTypeForParser() { + // no-op + } } }