This is an automated email from the ASF dual-hosted git repository.
gerlowskija pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new 81bb332cac8 SOLR-17256: Introduce SolrRequest.setBasePath alternative
(#2714)
81bb332cac8 is described below
commit 81bb332cac8aff530fc55466864e56c6e7c3fb27
Author: Jason Gerlowski <[email protected]>
AuthorDate: Fri Oct 4 12:23:31 2024 -0700
SOLR-17256: Introduce SolrRequest.setBasePath alternative (#2714)
Mutating SolrRequest objects as setBasePath does is a bad practice, as
the modification isn't documented and many users expect to reuse these
request objects across multiple requests.
This commit adds an alternative approach which allows both clients and
SolrRequest instances to remain immutable: a
`Http2SolrClient.requestWithBaseUrl` method.
setBasePath usages will be removed in a subsequent commit.
---------
Co-authored-by: David Smiley <[email protected]>
---
solr/CHANGES.txt | 4 ++
.../java/org/apache/solr/handler/IndexFetcher.java | 31 +++++------
.../solr/client/solrj/SolrClientFunction.java | 25 +++++++++
.../solr/client/solrj/impl/Http2SolrClient.java | 62 +++++++++++++++++++++-
.../apache/solr/client/solrj/util/ClientUtils.java | 2 +
.../solr/client/solrj/impl/DebugServlet.java | 3 ++
.../client/solrj/impl/Http2SolrClientTest.java | 33 ++++++++++++
7 files changed, 140 insertions(+), 20 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index aa616708933..f4f3893ed2f 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -27,6 +27,10 @@ Improvements
* SOLR-17442: Resolve -v flag conflict (version, value, verbose) in bin/solr.
(Eric Pugh, Christos Malliaridis)
+* SOLR-17256: Deprecate SolrRequest `setBasePath` and `getBasePath` methods.
SolrJ users wishing to temporarily
+ override an HTTP client's base URL may use
`Http2SolrClient.requestWithBaseUrl` instead. (Jason Gerlowski,
+ Sanjay Dutt, David Smiley)
+
Optimizations
---------------------
* SOLR-14985: Solrj CloudSolrClient with Solr URLs had serious performance
regressions (since the
diff --git a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
index d6eaa232c4e..0826f40f3dc 100644
--- a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
+++ b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
@@ -95,7 +95,6 @@ import org.apache.lucene.store.FilterDirectory;
import org.apache.lucene.store.IOContext;
import org.apache.lucene.store.IndexInput;
import org.apache.lucene.store.IndexOutput;
-import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.impl.Http2SolrClient;
import org.apache.solr.client.solrj.impl.HttpClientUtil;
@@ -186,7 +185,7 @@ public class IndexFetcher {
boolean fetchFromLeader = false;
- private final SolrClient solrClient;
+ private final Http2SolrClient solrClient;
private Integer connTimeout;
@@ -264,17 +263,15 @@ public class IndexFetcher {
// It's crucial not to remove the authentication credentials as they are
essential for User
// managed replication.
// GitHub PR #2276
- private SolrClient createSolrClient(
+ private Http2SolrClient createSolrClient(
SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword,
String leaderBaseUrl) {
final UpdateShardHandler updateShardHandler =
core.getCoreContainer().getUpdateShardHandler();
- Http2SolrClient httpClient =
- new Http2SolrClient.Builder(leaderBaseUrl)
- .withHttpClient(updateShardHandler.getRecoveryOnlyHttpClient())
- .withBasicAuthCredentials(httpBasicAuthUser, httpBasicAuthPassword)
- .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
- .withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS)
- .build();
- return httpClient;
+ return new Http2SolrClient.Builder(leaderBaseUrl)
+ .withHttpClient(updateShardHandler.getRecoveryOnlyHttpClient())
+ .withBasicAuthCredentials(httpBasicAuthUser, httpBasicAuthPassword)
+ .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
+ .withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS)
+ .build();
}
public IndexFetcher(
@@ -377,10 +374,8 @@ public class IndexFetcher {
params.set(CommonParams.WT, JAVABIN);
params.set(CommonParams.QT, ReplicationHandler.PATH);
QueryRequest req = new QueryRequest(params);
- req.setBasePath(leaderBaseUrl);
- // TODO modify to use shardhandler
try {
- return solrClient.request(req, leaderCoreName);
+ return solrClient.requestWithBaseUrl(leaderBaseUrl, leaderCoreName,
req).getResponse();
} catch (SolrServerException e) {
throw new SolrException(ErrorCode.SERVER_ERROR, e.getMessage(), e);
}
@@ -398,10 +393,9 @@ public class IndexFetcher {
params.set(CommonParams.WT, JAVABIN);
params.set(CommonParams.QT, ReplicationHandler.PATH);
QueryRequest req = new QueryRequest(params);
- req.setBasePath(leaderBaseUrl);
- // TODO modify to use shardhandler
try {
- NamedList<?> response = solrClient.request(req, leaderCoreName);
+ NamedList<?> response =
+ solrClient.requestWithBaseUrl(leaderBaseUrl, leaderCoreName,
req).getResponse();
List<Map<String, Object>> files = (List<Map<String, Object>>)
response.get(CMD_GET_FILE_LIST);
if (files != null) filesToDownload = Collections.synchronizedList(files);
@@ -2135,9 +2129,8 @@ public class IndexFetcher {
params.set(CommonParams.QT, ReplicationHandler.PATH);
QueryRequest request = new QueryRequest(params);
- request.setBasePath(leaderBaseUrl);
// TODO use shardhandler
- return solrClient.request(request, leaderCoreName);
+ return solrClient.requestWithBaseUrl(leaderBaseUrl, leaderCoreName,
request).getResponse();
}
public void destroy() {
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
new file mode 100644
index 00000000000..0adb49471dc
--- /dev/null
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClientFunction.java
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj;
+
+import java.io.IOException;
+
+/** A lambda intended for invoking SolrClient operations */
+@FunctionalInterface
+public interface SolrClientFunction<C extends SolrClient, R> {
+ R apply(C c) throws IOException, SolrServerException;
+}
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 b7823db6b9b..36f8bfcf9b2 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
@@ -41,7 +41,9 @@ import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;
import org.apache.solr.client.solrj.ResponseParser;
+import org.apache.solr.client.solrj.SolrClientFunction;
import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrResponse;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.V2RequestSupport;
import org.apache.solr.client.solrj.embedded.SSLConfig;
@@ -117,7 +119,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
private final HttpClient httpClient;
private List<HttpListenerFactory> listenerFactory = new ArrayList<>();
- private final AsyncTracker asyncTracker = new AsyncTracker();
+ protected AsyncTracker asyncTracker = new AsyncTracker();
private final boolean closeClient;
private ExecutorService executor;
@@ -558,6 +560,41 @@ public class Http2SolrClient extends HttpSolrClientBase {
}
}
+ /**
+ * Executes a SolrRequest using the provided URL to temporarily override any
"base URL" currently
+ * used by this client
+ *
+ * @param baseUrl a URL to a root Solr path (i.e. "/solr") that should be
used for this request
+ * @param collection an optional collection or core name used to override
the client's "default
+ * collection". May be 'null' for any requests that don't require a
collection or wish to rely
+ * on the client's default
+ * @param req the SolrRequest to send
+ */
+ public final <R extends SolrResponse> R requestWithBaseUrl(
+ String baseUrl, String collection, SolrRequest<R> req)
+ throws SolrServerException, IOException {
+ return requestWithBaseUrl(baseUrl, (c) -> req.process(c, collection));
+ }
+
+ /**
+ * Temporarily modifies the client to use a different base URL and runs the
provided lambda
+ *
+ * @param baseUrl the base URL to use on any requests made within the
'clientFunction' lambda
+ * @param clientFunction a Function that consumes a Http2SolrClient and
returns an arbitrary value
+ * @return the value returned after invoking 'clientFunction'
+ * @param <R> the type returned by the provided function (and by this method)
+ */
+ public <R> R requestWithBaseUrl(
+ String baseUrl, SolrClientFunction<Http2SolrClient, R> clientFunction)
+ throws SolrServerException, IOException {
+
+ // Despite the name, try-with-resources used here to avoid IDE and
ObjectReleaseTracker
+ // complaints
+ try (final var derivedClient = new NoCloseHttp2SolrClient(baseUrl, this)) {
+ return clientFunction.apply(derivedClient);
+ }
+ }
+
private NamedList<Object> processErrorsAndResponse(
SolrRequest<?> solrRequest, Response response, InputStream is, String
urlExceptionMessage)
throws SolrServerException {
@@ -846,6 +883,29 @@ public class Http2SolrClient extends HttpSolrClientBase {
return serverBaseUrl;
}
+ /**
+ * An Http2SolrClient that doesn't close or cleanup any resources
+ *
+ * <p>Only safe to use as a derived copy of an existing instance which
retains responsibility for
+ * closing all involved resources.
+ *
+ * @see #requestWithBaseUrl(String, SolrClientFunction)
+ */
+ private static class NoCloseHttp2SolrClient extends Http2SolrClient {
+
+ public NoCloseHttp2SolrClient(String baseUrl, Http2SolrClient
parentClient) {
+ super(baseUrl, new
Http2SolrClient.Builder(baseUrl).withHttpClient(parentClient));
+
+ this.asyncTracker = parentClient.asyncTracker;
+ }
+
+ @Override
+ public void close() {
+ /* Intentional no-op */
+ ObjectReleaseTracker.release(this);
+ }
+ }
+
private static class AsyncTracker {
private static final int MAX_OUTSTANDING_REQUESTS = 1000;
diff --git
a/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java
b/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java
index ac2c5f76ed2..37af22f8721 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java
@@ -82,6 +82,8 @@ public class ClientUtils {
String serverRootUrl,
String collection)
throws MalformedURLException {
+
+ // TODO remove getBasePath support here prior to closing SOLR-17256
String basePath = solrRequest.getBasePath() == null ? serverRootUrl :
solrRequest.getBasePath();
if (solrRequest.getApiVersion() == SolrRequest.ApiVersion.V2) {
diff --git
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/DebugServlet.java
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/DebugServlet.java
index 9cfc31c31f8..a37d64476d5 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/DebugServlet.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/DebugServlet.java
@@ -34,6 +34,7 @@ import org.apache.solr.common.util.SuppressForbidden;
public class DebugServlet extends HttpServlet {
public static void clear() {
lastMethod = null;
+ url = null;
headers = null;
parameters = null;
errorCode = null;
@@ -45,6 +46,7 @@ public class DebugServlet extends HttpServlet {
public static Integer errorCode = null;
public static String lastMethod = null;
+ public static String url = null;
public static HashMap<String, String> headers = null;
public static Map<String, String[]> parameters = null;
public static String queryString = null;
@@ -123,6 +125,7 @@ public class DebugServlet extends HttpServlet {
}
private void recordRequest(HttpServletRequest req, HttpServletResponse resp)
{
+ url = req.getRequestURL().toString();
setHeaders(req);
setParameters(req);
setQueryString(req);
diff --git
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
index 7217969ce76..0e6ee7bb8a2 100644
---
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
+++
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
@@ -32,6 +32,7 @@ import org.apache.solr.client.solrj.request.QueryRequest;
import org.apache.solr.client.solrj.request.RequestWriter;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.MapSolrParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.util.ssl.SslContextFactory;
@@ -181,6 +182,38 @@ public class Http2SolrClientTest extends
HttpSolrClientTestBase {
super.testQueryXmlPut();
}
+ @Test
+ public void testOverrideBaseUrl() throws Exception {
+ DebugServlet.clear();
+ final var defaultUrl =
+ "http://not-a-real-url:8983/solr";; // Would result in an exception if
used
+ final var baseUrlToUse = getBaseUrl() + DEBUG_SERVLET_PATH;
+ final var queryParams = new ModifiableSolrParams();
+ queryParams.add("q", "*:*");
+
+ // Ensure the correct URL is used by the lambda-based requestWithBaseUrl
method
+ try (Http2SolrClient client =
+ new
Http2SolrClient.Builder(defaultUrl).withDefaultCollection(DEFAULT_CORE).build())
{
+ try {
+ client.requestWithBaseUrl(baseUrlToUse, (c) -> c.query(queryParams));
+ } catch (BaseHttpSolrClient.RemoteSolrException rse) {
+ }
+
+ assertEquals(baseUrlToUse + "/" + DEFAULT_CORE + "/select",
DebugServlet.url);
+ }
+
+ // Ensure the correct URL is used by the SolrRequest-based
requestWithBaseUrl method
+ try (Http2SolrClient client =
+ new
Http2SolrClient.Builder(defaultUrl).withDefaultCollection(DEFAULT_CORE).build())
{
+ try {
+ client.requestWithBaseUrl(baseUrlToUse, null, new
QueryRequest(queryParams));
+ } catch (BaseHttpSolrClient.RemoteSolrException rse) {
+ }
+
+ assertEquals(baseUrlToUse + "/" + DEFAULT_CORE + "/select",
DebugServlet.url);
+ }
+ }
+
@Test
public void testDelete() throws Exception {
DebugServlet.clear();
