gerlowskija commented on code in PR #3881:
URL: https://github.com/apache/solr/pull/3881#discussion_r2573753792
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -506,10 +506,8 @@ private NamedList<Object> doRequest(
// 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);
+ if (solrClient instanceof HttpSolrClientBase hasReqWithUrl) {
Review Comment:
[0] Comment block above this line could probably be updated to reference
HttpSolrClientBase instead of Http2SolrClient
[+1] The code itself here is way cleaner ; love it.
##########
changelog/unreleased/SOLR-17996-requestWithBaseUrl.yml:
##########
@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: New SolrJ SolrRequest.requestWithBaseUrl, new
HttpSolrClientBase.requestWithBaseUrl
Review Comment:
[-0] Looks like this changelog has the wrong method name for the SolrRequest
method at least. Probably worth changing since it's user-facing?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -291,6 +292,15 @@ public final T process(SolrClient client, String
collection)
return typedResponse;
}
+ /**
+ * @lucene.experimental
+ */
+ public final T processWithBaseUrl(HttpSolrClientBase client, String url,
String collection)
Review Comment:
[-0] Would be great if this method had Javadocs with some details about the
method parameters: what sort of baseUrl is expected, is the "collection"
parameter required, etc.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -613,20 +609,11 @@ public NamedList<Object> request(SolrRequest<?>
solrRequest, String collection)
}
}
- /**
- * 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)
+ @Override
+ public NamedList<Object> requestWithBaseUrl(
Review Comment:
> This is a backwards incompatible signature change.
@dsmiley - wdyt about adding something to "Upgrade Notes", or even flushing
out the changelog entry a bit to flag this more visibly for users since they
we're not giving them the standard "deprecation" period on the method removal?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
