From 704f2fe3f72fd2402d14d16c2f9d6e866869723d Mon Sep 17 00:00:00 2001 From: jdyer1 Date: Mon, 23 Dec 2024 17:41:16 -0600 Subject: [PATCH] Revert "SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics (#2899)" This reverts commit d3e57aad8259f4b9c574fbff5cb5f6d112bef85a. --- solr/CHANGES.txt | 14 +- .../org/apache/solr/cloud/ZkController.java | 11 +- .../solr/core/HttpSolrClientProvider.java | 13 +- .../handler/component/HttpShardHandler.java | 2 +- .../component/HttpShardHandlerFactory.java | 15 +- .../security/HttpClientBuilderPlugin.java | 5 - .../security/PKIAuthenticationPlugin.java | 12 +- .../org/apache/solr/cli/PostToolTest.java | 1 - .../org/apache/solr/cloud/OverseerTest.java | 9 +- .../solr/client/solrj/SolrClientFunction.java | 6 +- .../solrj/impl/CloudHttp2SolrClient.java | 45 +++- .../client/solrj/impl/Http2SolrClient.java | 36 +-- .../client/solrj/impl/HttpJdkSolrClient.java | 24 +- .../client/solrj/impl/LBHttp2SolrClient.java | 116 +++----- .../solr/client/solrj/impl/LBSolrClient.java | 32 ++- .../impl/CloudHttp2SolrClientBuilderTest.java | 56 +++- .../solrj/impl/HttpJdkSolrClientTest.java | 20 ++ .../LBHttp2SolrClientIntegrationTest.java | 28 +- .../solrj/impl/LBHttp2SolrClientTest.java | 247 +++++++----------- 19 files changed, 348 insertions(+), 344 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index a135ecaa539..a844220f2fc 100644 --- a/solr/CHANGES.txt +++ b/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 --------------------- 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 5a890121a43..7dd30a14d24 100644 --- a/solr/core/src/java/org/apache/solr/cloud/ZkController.java +++ b/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(); 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 e9631b26d1f..2bf25a896f6 100644 --- a/solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java +++ b/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 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 320a5fe70d7..7592eed86fc 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 1437dee63ea..ac7dc0cf8e0 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,9 +83,8 @@ 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; @@ -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(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); } } 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 206fb3d0886..b43d5f22190 100644 --- a/solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java +++ b/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); - } } 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 93ecdcc9d68..b1f6e6b6eed 100644 --- a/solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java +++ b/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 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 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 9eb3e783a2d..758ae9ccd51 100644 --- a/solr/core/src/test/org/apache/solr/cli/PostToolTest.java +++ b/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"); 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 e611902898f..bad8d58d021 100644 --- a/solr/core/src/test/org/apache/solr/cloud/OverseerTest.java +++ b/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; 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 68246001a40..0adb49471dc 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,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 { 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 18aa318f8ba..ade1ebe433f 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,8 +40,9 @@ 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 @@ -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(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 getLbClient() { + public LBHttp2SolrClient getLbClient() { return lbClient; } @@ -168,6 +171,7 @@ 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; @@ -400,6 +404,23 @@ 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 @@ -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; } 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 fddc4e2daa6..fb2eb1a123f 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,7 +140,9 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) { this.httpClient = createHttpClient(builder); this.closeClient = true; } - this.listenerFactory.addAll(builder.listenerFactories); + if (builder.listenerFactory != null) { + this.listenerFactory.addAll(builder.listenerFactory); + } updateDefaultMimeTypeForParser(); this.httpClient.setFollowRedirects(Boolean.TRUE.equals(builder.followRedirects)); @@ -569,7 +571,6 @@ 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) @@ -905,7 +906,7 @@ public static class Builder protected Long keyStoreReloadIntervalSecs; - private List listenerFactories = new ArrayList<>(0); + private List listenerFactory; public Builder() { super(); @@ -931,27 +932,8 @@ public Builder(String baseSolrUrl) { this.baseSolrUrl = baseSolrUrl; } - /** - * 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); + public Http2SolrClient.Builder withListenerFactory(List listenerFactory) { + this.listenerFactory = listenerFactory; return this; } @@ -1127,9 +1109,9 @@ public Builder withHttpClient(Http2SolrClient http2SolrClient) { if (this.urlParamNames == null) { this.urlParamNames = http2SolrClient.urlParamNames; } - if (this.listenerFactories.isEmpty()) { - this.listenerFactories.clear(); - http2SolrClient.listenerFactory.forEach(this.listenerFactories::add); + if (this.listenerFactory == null) { + this.listenerFactory = new ArrayList(); + http2SolrClient.listenerFactory.forEach(this.listenerFactory::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 c26d13606b6..1127b3fd1a1 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); + PreparedRequest pReq = prepareRequest(solrRequest, collection, null); return httpClient .sendAsync(pReq.reqb.build(), HttpResponse.BodyHandlers.ofInputStream()) .thenApply( @@ -157,10 +157,10 @@ public CompletableFuture> requestAsync( } } - @Override - public NamedList request(SolrRequest solrRequest, String collection) + protected NamedList requestWithBaseUrl( + String baseUrl, SolrRequest solrRequest, String collection) throws SolrServerException, IOException { - PreparedRequest pReq = prepareRequest(solrRequest, collection); + PreparedRequest pReq = prepareRequest(solrRequest, collection, baseUrl); HttpResponse response = null; try { response = httpClient.send(pReq.reqb.build(), HttpResponse.BodyHandlers.ofInputStream()); @@ -192,13 +192,25 @@ public NamedList request(SolrRequest solrRequest, String collection) } } - private PreparedRequest prepareRequest(SolrRequest solrRequest, String collection) + @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) throws SolrServerException, IOException { checkClosed(); if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) { collection = defaultCollection; } - String url = getRequestUrl(solrRequest, collection); + String url; + if (overrideBaseUrl != null) { + url = ClientUtils.buildRequestUrl(solrRequest, overrideBaseUrl, collection); + } else { + 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 4c0d46b13db..2c926a26261 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,25 +23,22 @@ 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 balancer for multiple Http Solr Clients. 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 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. * *

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. @@ -59,7 +56,7 @@ *

* *
- * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClientBuilder,
+ * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClient,
  *         new LBSolrClient.Endpoint("http://host1:8080/solr"), new LBSolrClient.Endpoint("http://host2:8080/solr"))
  *     .build();
  * 
@@ -72,7 +69,7 @@ *
* *
- * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClientBuilder,
+ * SolrClient client = new LBHttp2SolrClient.Builder(http2SolrClient,
  *         new LBSolrClient.Endpoint("http://host1:8080/solr", "coreA"),
  *         new LBSolrClient.Endpoint("http://host2:8080/solr", "coreB"))
  *     .build();
@@ -97,63 +94,35 @@
  *
  * @since solr 8.0
  */
-public class LBHttp2SolrClient> extends LBSolrClient {
+public class LBHttp2SolrClient extends LBSolrClient {
 
-  private final Map urlToClient;
-  private final Set urlParamNames;
-
-  // must synchronize on this when building
-  private final HttpSolrClientBuilderBase solrClientBuilder;
+  protected final C solrClient;
 
+  @SuppressWarnings("unchecked")
   private LBHttp2SolrClient(Builder builder) {
     super(Arrays.asList(builder.solrEndpoints));
-    this.solrClientBuilder = builder.solrClientBuilder;
-
+    this.solrClient = (C) builder.solrClient;
     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 HttpSolrClientBase getClient(final Endpoint endpoint) {
-    return urlToClient.computeIfAbsent(
-        endpoint.getBaseUrl(),
-        url -> {
-          synchronized (solrClientBuilder) {
-            solrClientBuilder.baseSolrUrl = url;
-            return solrClientBuilder.build();
-          }
-        });
+  protected SolrClient getClient(Endpoint endpoint) {
+    return solrClient;
   }
 
   @Override
   public ResponseParser getParser() {
-    return urlToClient.isEmpty() ? null : urlToClient.values().iterator().next().getParser();
+    return solrClient.getParser();
   }
 
   @Override
   public RequestWriter getRequestWriter() {
-    return urlToClient.isEmpty() ? null : urlToClient.values().iterator().next().getRequestWriter();
+    return solrClient.getRequestWriter();
   }
 
   public Set getUrlParamNames() {
-    return urlParamNames;
-  }
-
-  @Override
-  public void close() {
-    urlToClient.values().forEach(IOUtils::closeQuietly);
-    super.close();
+    return solrClient.getUrlParamNames();
   }
 
   /**
@@ -241,18 +210,23 @@ private CompletableFuture> doAsyncRequest(
       RetryListener listener) {
     String baseUrl = endpoint.toString();
     rsp.server = baseUrl;
-    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;
+    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);
+    }
   }
 
   private void onSuccessfulRequest(
@@ -316,28 +290,16 @@ private void onFailedRequest(
     }
   }
 
-  public static class Builder> {
-
-    private final B solrClientBuilder;
+  public static class Builder {
 
+    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;
 
-    /**
-     * 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; + public Builder(C solrClient, Endpoint... endpoints) { + this.solrClient = solrClient; this.solrEndpoints = endpoints; } @@ -347,7 +309,7 @@ public Builder(B solrClientBuilder, 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); @@ -357,13 +319,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 31c75662b48..64201b03c13 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,7 +497,25 @@ private static boolean isTimeExceeded(long timeAllowedNano, long timeOutTime) { private NamedList doRequest(Endpoint endpoint, SolrRequest solrRequest) throws SolrServerException, IOException { final var solrClient = getClient(endpoint); - return solrClient.request(solrRequest, endpoint.getCore()); + 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); } protected Exception doRequest( @@ -607,7 +625,12 @@ 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 = getClient(zombieEndpoint).request(queryRequest, effectiveCollection); + final var responseRaw = + doRequest( + getClient(zombieEndpoint), + zombieEndpoint.getBaseUrl(), + effectiveCollection, + queryRequest); QueryResponse resp = new QueryResponse(); resp.setResponse(responseRaw); @@ -711,7 +734,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 getClient(endpoint).request(request, effectiveCollection); + return doRequest(getClient(endpoint), endpoint.getBaseUrl(), effectiveCollection, request); } catch (SolrException e) { // Server is alive but the request was malformed or invalid throw e; @@ -752,7 +775,8 @@ public NamedList request( ++numServersTried; final String effectiveCollection = endpoint.getCore() == null ? collection : endpoint.getCore(); - NamedList rsp = getClient(endpoint).request(request, effectiveCollection); + NamedList rsp = + doRequest(getClient(endpoint), endpoint.getBaseUrl(), effectiveCollection, request); // 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 46b6883f755..0846dfefc6c 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,6 +120,26 @@ 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); @@ -139,6 +159,20 @@ 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 = @@ -161,19 +195,29 @@ public void testDefaultCollectionPassedFromBuilderToClient() throws IOException public void testHttpClientPreservedInHttp2ClusterStateProvider() throws IOException { List solrUrls = List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString()); - // without internalClientBuilder - testHttpClientConsistency(solrUrls, null); + // No httpClient - No internalClientBuilder + testHttpClientConsistency(solrUrls, null, null); + + // httpClient - No internalClientBuilder + try (Http2SolrClient httpClient = new Http2SolrClient.Builder().build()) { + testHttpClientConsistency(solrUrls, httpClient, null); + } - // with internalClientBuilder + // No httpClient - internalClientBuilder Http2SolrClient.Builder internalClientBuilder = new Http2SolrClient.Builder(); - testHttpClientConsistency(solrUrls, internalClientBuilder); + testHttpClientConsistency(solrUrls, null, internalClientBuilder); } private void testHttpClientConsistency( - List solrUrls, Http2SolrClient.Builder internalClientBuilder) throws IOException { + List solrUrls, + Http2SolrClient httpClient, + Http2SolrClient.Builder internalClientBuilder) + throws IOException { CloudHttp2SolrClient.Builder clientBuilder = new CloudHttp2SolrClient.Builder(solrUrls); - if (internalClientBuilder != null) { + if (httpClient != null) { + clientBuilder.withHttpClient(httpClient); + } else 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 1dbfc0e998b..b3980ad44bc 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,6 +34,7 @@ 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; @@ -153,6 +154,25 @@ 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 9c2f79ff0a5..61504a052b8 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,6 +18,7 @@ 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; @@ -117,28 +118,30 @@ public void tearDown() throws Exception { private LBClientHolder client(LBSolrClient.Endpoint... baseSolrEndpoints) { if (random().nextBoolean()) { - var delegateClientBuilder = + var delegateClient = new Http2SolrClient.Builder() .withConnectionTimeout(1000, TimeUnit.MILLISECONDS) - .withIdleTimeout(2000, TimeUnit.MILLISECONDS); + .withIdleTimeout(2000, TimeUnit.MILLISECONDS) + .build(); var lbClient = - new LBHttp2SolrClient.Builder<>(delegateClientBuilder, baseSolrEndpoints) + new LBHttp2SolrClient.Builder<>(delegateClient, baseSolrEndpoints) .withDefaultCollection(solr[0].getDefaultCollection()) .setAliveCheckInterval(500, TimeUnit.MILLISECONDS) .build(); - return new LBClientHolder(lbClient, delegateClientBuilder); + return new LBClientHolder(lbClient, delegateClient); } else { - var delegateClientBuilder = + var delegateClient = new HttpJdkSolrClient.Builder() .withConnectionTimeout(1000, TimeUnit.MILLISECONDS) .withIdleTimeout(2000, TimeUnit.MILLISECONDS) - .withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT); + .withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT) + .build(); var lbClient = - new LBHttp2SolrClient.Builder<>(delegateClientBuilder, baseSolrEndpoints) + new LBHttp2SolrClient.Builder<>(delegateClient, baseSolrEndpoints) .withDefaultCollection(solr[0].getDefaultCollection()) .setAliveCheckInterval(500, TimeUnit.MILLISECONDS) .build(); - return new LBClientHolder(lbClient, delegateClientBuilder); + return new LBClientHolder(lbClient, delegateClient); } } @@ -314,9 +317,9 @@ public void startJetty() throws Exception { private static class LBClientHolder implements AutoCloseable { final LBHttp2SolrClient lbClient; - final HttpSolrClientBuilderBase delegate; + final HttpSolrClientBase delegate; - LBClientHolder(LBHttp2SolrClient lbClient, HttpSolrClientBuilderBase delegate) { + LBClientHolder(LBHttp2SolrClient lbClient, HttpSolrClientBase delegate) { this.lbClient = lbClient; this.delegate = delegate; } @@ -324,6 +327,11 @@ 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 30cee0804b5..9d2019309b0 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,8 +18,6 @@ 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; @@ -30,6 +28,7 @@ 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; @@ -52,12 +51,11 @@ public void testLBHttp2SolrClientWithTheseParamNamesInTheUrl() { Set urlParamNames = new HashSet<>(2); urlParamNames.add("param1"); - var httpSolrClientBuilder = - new Http2SolrClient.Builder(url).withTheseParamNamesInTheUrl(urlParamNames); - var endpoint = new LBSolrClient.Endpoint(url); - try (var testClient = - new LBHttp2SolrClient.Builder(httpSolrClientBuilder, endpoint) - .build()) { + try (Http2SolrClient http2SolrClient = + new Http2SolrClient.Builder(url).withTheseParamNamesInTheUrl(urlParamNames).build(); + LBHttp2SolrClient testClient = + new LBHttp2SolrClient.Builder<>(http2SolrClient, new LBSolrClient.Endpoint(url)) + .build()) { assertArrayEquals( "Wrong urlParamNames found in lb client.", @@ -66,7 +64,7 @@ public void testLBHttp2SolrClientWithTheseParamNamesInTheUrl() { assertArrayEquals( "Wrong urlParamNames found in base client.", urlParamNames.toArray(), - testClient.getClient(endpoint).getUrlParamNames().toArray()); + http2SolrClient.getUrlParamNames().toArray()); } } @@ -76,11 +74,12 @@ public void testSynchronous() throws Exception { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - var httpSolrClientBuilder = - new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); - - try (LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { + 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()) { String lastEndpoint = null; for (int i = 0; i < 10; i++) { @@ -104,14 +103,15 @@ public void testSynchronousWithFalures() throws Exception { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - var httpSolrClientBuilder = - new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); - - try (LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { + 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()) { - setEndpointToFail(testClient, ep1); - setEndpointToSucceed(testClient, ep2); + client.basePathToFail = ep1.getBaseUrl(); + String basePathToSucceed = ep2.getBaseUrl(); 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, - ep2.getBaseUrl(), + basePathToSucceed, response.server); checkSynchonousResponseContent(response, qValue); } - setEndpointToFail(testClient, ep2); - setEndpointToSucceed(testClient, ep1); + client.basePathToFail = ep2.getBaseUrl(); + basePathToSucceed = ep1.getBaseUrl(); qValue = "Second time"; for (int i = 0; i < 5; i++) { @@ -137,13 +137,21 @@ public void testSynchronousWithFalures() throws Exception { LBSolrClient.Rsp response = testClient.request(req); assertEquals( "The healthy node 'endpoint one' should have served the request: " + i, - ep1.getBaseUrl(), + basePathToSucceed, 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() { @@ -154,35 +162,28 @@ public void testAsyncWithFailures() { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - var httpSolrClientBuilder = - new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); - - try (LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { + 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()) { for (int j = 0; j < 2; j++) { // first time Endpoint One will return error code 500. // second time Endpoint One will be healthy - LBSolrClient.Endpoint endpointToSucceed; - LBSolrClient.Endpoint endpointToFail; + String basePathToSucceed; if (j == 0) { - setEndpointToFail(testClient, ep1); - setEndpointToSucceed(testClient, ep2); - endpointToSucceed = ep2; - endpointToFail = ep1; + client.basePathToFail = ep1.getBaseUrl(); + basePathToSucceed = ep2.getBaseUrl(); } else { - setEndpointToFail(testClient, ep2); - setEndpointToSucceed(testClient, ep1); - endpointToSucceed = ep1; - endpointToFail = ep2; + client.basePathToFail = ep2.getBaseUrl(); + basePathToSucceed = ep1.getBaseUrl(); } - List successEndpointLastBasePaths = - basePathsForEndpoint(testClient, endpointToSucceed); - List failEndpointLastBasePaths = basePathsForEndpoint(testClient, endpointToFail); for (int i = 0; i < 10; i++) { - // i: we'll try 10 times. It should behave the same with iter 2-10. . + // i: we'll try 10 times to see if it behaves the same every time. QueryRequest queryRequest = new QueryRequest(new MapSolrParams(Map.of("q", "" + i))); LBSolrClient.Req req = new LBSolrClient.Req(queryRequest, endpointList); @@ -195,26 +196,26 @@ public void testAsyncWithFailures() { } catch (TimeoutException | ExecutionException e) { fail(iterMessage + " Response ended in failure: " + e); } - if (i == 0) { - // When i=0, it must try both endpoints to find success: + // 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". // - // 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()); + // 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); } else { - // 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()); + // The first endpoint does not give the exception, it doesn't retry. + assertEquals(iterMessage, 1, client.lastBasePaths.size()); } - successEndpointLastBasePaths.clear(); - failEndpointLastBasePaths.clear(); + String successBasePath = client.lastBasePaths.remove(0); + assertEquals(iterMessage, basePathToSucceed, successBasePath); } } } @@ -226,11 +227,11 @@ public void testAsync() { LBSolrClient.Endpoint ep2 = new LBSolrClient.Endpoint("http://endpoint.two"); List endpointList = List.of(ep1, ep2); - var httpSolrClientBuilder = - new MockHttpSolrClientBuilder().withConnectionTimeout(10, TimeUnit.SECONDS); - - try (LBHttp2SolrClient testClient = - new LBHttp2SolrClient.Builder<>(httpSolrClientBuilder, ep1, ep2).build()) { + 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()) { int limit = 10; // For simplicity use an even limit List> responses = new ArrayList<>(); @@ -242,17 +243,23 @@ public void testAsync() { } QueryRequest[] queryRequests = new QueryRequest[limit]; - List> lastSolrRequests = lastSolrRequests(testClient, ep1, ep2); - assertEquals(limit, lastSolrRequests.size()); - + int numEndpointOne = 0; + int numEndpointTwo = 0; for (int i = 0; i < limit; i++) { - SolrRequest lastSolrReq = lastSolrRequests.get(i); + SolrRequest lastSolrReq = client.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(); @@ -271,55 +278,15 @@ 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! - 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, numEndpointOne); + assertEquals(0, numEndpointTwo); - 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); + assertEquals(limit, client.lastSolrRequests.size()); + assertEquals(limit, client.lastCollections.size()); } } - public static class MockHttpSolrClient extends HttpSolrClientBase { + public static class MockHttpSolrClient extends Http2SolrClient { public List> lastSolrRequests = new ArrayList<>(); @@ -327,13 +294,15 @@ public static class MockHttpSolrClient extends HttpSolrClientBase { public List lastCollections = new ArrayList<>(); - public boolean allRequestsShallFail; + public String basePathToFail = null; public String tmpBaseUrl = null; - public boolean closeCalled; + protected MockHttpSolrClient(String serverBaseUrl, Builder builder) { - protected MockHttpSolrClient(String serverBaseUrl, MockHttpSolrClientBuilder builder) { + // TODO: Consider creating an interface for Http*SolrClient + // so mocks can Implement, not Extend, and not actually need to + // build an (unused) client super(serverBaseUrl, builder); } @@ -343,12 +312,25 @@ public NamedList request(final SolrRequest request, String collection lastSolrRequests.add(request); lastBasePaths.add(tmpBaseUrl); lastCollections.add(collection); - if (allRequestsShallFail) { + if (tmpBaseUrl.equals(basePathToFail)) { 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) { @@ -356,7 +338,7 @@ public CompletableFuture> requestAsync( lastSolrRequests.add(solrRequest); lastBasePaths.add(tmpBaseUrl); lastCollections.add(collection); - if (allRequestsShallFail) { + if (tmpBaseUrl != null && tmpBaseUrl.equals(basePathToFail)) { cf.completeExceptionally( new SolrException(SolrException.ErrorCode.SERVER_ERROR, "We should retry this.")); } else { @@ -369,32 +351,5 @@ 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 - } } }