dsmiley commented on code in PR #2811:
URL: https://github.com/apache/solr/pull/2811#discussion_r1823254387
##########
solr/core/src/test/org/apache/solr/handler/TestUserManagedReplicationWithAuth.java:
##########
@@ -227,26 +228,26 @@ private QueryResponse queryWithBasicAuth(SolrClient
client, SolrQuery q)
return withBasicAuth(new QueryRequest(q)).process(client);
}
- private void disablePoll(JettySolrRunner Jetty, SolrClient solrClient)
+ private void disablePoll(JettySolrRunner Jetty, Http2SolrClient solrClient)
throws SolrServerException, IOException {
ModifiableSolrParams disablePollParams = new ModifiableSolrParams();
disablePollParams.set(COMMAND, CMD_DISABLE_POLL);
disablePollParams.set(CommonParams.WT, JAVABIN);
disablePollParams.set(CommonParams.QT, ReplicationHandler.PATH);
QueryRequest req = new QueryRequest(disablePollParams);
withBasicAuth(req);
- req.setBasePath(buildUrl(Jetty.getLocalPort()));
- solrClient.request(req, DEFAULT_TEST_CORENAME);
+ final var baseUrl = buildUrl(Jetty.getLocalPort());
+ solrClient.requestWithBaseUrl(baseUrl, DEFAULT_TEST_CORENAME, req);
}
private void pullIndexFromTo(
JettySolrRunner srcSolr, JettySolrRunner destSolr, boolean authEnabled)
throws SolrServerException, IOException {
String srcUrl = buildUrl(srcSolr.getLocalPort()) + "/" +
DEFAULT_TEST_CORENAME;
- String destUrl = buildUrl(destSolr.getLocalPort()) + "/" +
DEFAULT_TEST_CORENAME;
QueryRequest req = getQueryRequestForFetchIndex(authEnabled, srcUrl);
- req.setBasePath(buildUrl(destSolr.getLocalPort()));
+ final var baseUrl = buildUrl(destSolr.getLocalPort());
+ followerClient.requestWithBaseUrl(baseUrl, DEFAULT_TEST_CORENAME, req);
followerClient.request(req, DEFAULT_TEST_CORENAME);
Review Comment:
remove
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -480,14 +480,35 @@ private static boolean isTimeExceeded(long
timeAllowedNano, long timeOutTime) {
return timeAllowedNano > 0 && System.nanoTime() > timeOutTime;
}
+ private NamedList<Object> doMakeRequest(Endpoint endpoint, SolrRequest<?>
solrRequest)
+ throws SolrServerException, IOException {
+ final var solrClient = getClient(endpoint);
+ return doMakeRequest(solrClient, endpoint.getBaseUrl(),
endpoint.getCore(), solrRequest);
+ }
+
+ // TODO This special casing can be removed if either: (1) SOLR-16367 is
completed, or (2)
+ // LBHttp2SolrClient.getClient() is modified to return a client already
pointed at the correct URL
+ private NamedList<Object> doMakeRequest(
+ 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) {
+ final var httpSolrClient = (Http2SolrClient) solrClient;
+ return httpSolrClient.requestWithBaseUrl(baseUrl, (c) ->
c.request(solrRequest, collection));
+ }
+
+ return solrClient.request(solrRequest, collection);
Review Comment:
then how is baseUrl applied? (after reading below) okay I see. A comment
directly above this line saying "assume solrClient is pointed at baseUrl" would
be very helpful.
Note there could be a JDK client specific issue as as @jdyer1 is presently
working on JDK client specific support.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -358,28 +358,29 @@ private void addRunner() {
@Override
public NamedList<Object> request(final SolrRequest<?> request, String
collection)
throws SolrServerException, IOException {
- if (ClientUtils.shouldApplyDefaultCollection(collection, request))
- collection = defaultCollection;
+ final String effectiveCollection =
+ ClientUtils.shouldApplyDefaultCollection(collection, request)
+ ? defaultCollection
+ : collection;
Review Comment:
why not update collection instead of having both "collection" and
"effectiveCollection" in scope, which risks accidental use of one over the other
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -687,11 +715,11 @@ public NamedList<Object> request(
final var endpoint = wrapper.getEndpoint();
try {
++numServersTried;
- request.setBasePath(endpoint.getBaseUrl());
// 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 doMakeRequest(
+ getClient(endpoint), endpoint.getBaseUrl(), effectiveCollection,
request);
Review Comment:
use overload
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -721,10 +749,10 @@ public NamedList<Object> request(
|| (justFailed != null &&
justFailed.containsKey(endpoint.getUrl()))) continue;
try {
++numServersTried;
- request.setBasePath(endpoint.getBaseUrl());
final String effectiveCollection =
endpoint.getCore() == null ? collection : endpoint.getCore();
- NamedList<Object> rsp = getClient(endpoint).request(request,
effectiveCollection);
+ NamedList<Object> rsp =
+ doMakeRequest(getClient(endpoint), endpoint.getBaseUrl(),
effectiveCollection, request);
Review Comment:
use overload
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -585,11 +606,18 @@ private void checkAZombieServer(EndpointWrapper
zombieServer) {
final Endpoint zombieEndpoint = zombieServer.getEndpoint();
try {
QueryRequest queryRequest = new QueryRequest(solrQuery);
- queryRequest.setBasePath(zombieEndpoint.getBaseUrl());
// First the one on the endpoint, then the default collection
final String effectiveCollection =
Objects.requireNonNullElse(zombieEndpoint.getCore(),
getDefaultCollection());
- QueryResponse resp = queryRequest.process(getClient(zombieEndpoint),
effectiveCollection);
+ final var responseRaw =
+ doMakeRequest(
+ getClient(zombieEndpoint),
+ zombieEndpoint.getBaseUrl(),
Review Comment:
use overload
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -205,21 +210,36 @@ public static class MockHttp2SolrClient extends
Http2SolrClient {
public String basePathToFail = null;
+ public String tmpBaseUrl = null;
+
protected MockHttp2SolrClient(String serverBaseUrl, Builder 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);
}
+ @Override
+ public <R> R requestWithBaseUrl(
+ String baseUrl, SolrClientFunction<Http2SolrClient, R> clientFunction)
+ throws SolrServerException, IOException {
+ // This use of 'tmpBaseUrl' is thread unsafe, but that's fine for our
purposes here.
Review Comment:
"NOT thread-safe" would read better
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -480,14 +480,35 @@ private static boolean isTimeExceeded(long
timeAllowedNano, long timeOutTime) {
return timeAllowedNano > 0 && System.nanoTime() > timeOutTime;
}
+ private NamedList<Object> doMakeRequest(Endpoint endpoint, SolrRequest<?>
solrRequest)
+ throws SolrServerException, IOException {
+ final var solrClient = getClient(endpoint);
+ return doMakeRequest(solrClient, endpoint.getBaseUrl(),
endpoint.getCore(), solrRequest);
+ }
+
+ // TODO This special casing can be removed if either: (1) SOLR-16367 is
completed, or (2)
+ // LBHttp2SolrClient.getClient() is modified to return a client already
pointed at the correct URL
+ private NamedList<Object> doMakeRequest(
Review Comment:
This method name feels awkward, wouldn't a method named requestWithBaseUrl
be clearer and consistent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
