Copilot commented on code in PR #3774:
URL: https://github.com/apache/solr/pull/3774#discussion_r2434241969
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -379,24 +380,31 @@ protected boolean maybeTryHeadRequest(String url) {
if (forceHttp11 || url == null ||
url.toLowerCase(Locale.ROOT).startsWith("https://";)) {
return true;
}
- return maybeTryHeadRequestSync(url);
- }
-
- protected volatile boolean headRequested; // must be threadsafe
- private boolean headSucceeded; // must be threadsafe
-
- private synchronized boolean maybeTryHeadRequestSync(String url) {
- if (headRequested) {
- return headSucceeded;
- }
-
URI uriNoQueryParams;
try {
- uriNoQueryParams = new URI(url);
+ var uriWithParams = new URI(url);
+ var uriNoPQueryParamsStr =
+ uriWithParams.getScheme()
+ + "://"
+ + uriWithParams.getAuthority()
+ + uriWithParams.getPath();
+ uriNoQueryParams = new URI(uriNoPQueryParamsStr);
Review Comment:
If uriWithParams.getPath() returns null (e.g. for a base URL without a
path), concatenation will yield the literal 'null' and produce an invalid URI;
default to empty string when path is null. Additionally, constructing the base
URL manually risks missing user-info or other components; consider using URI's
constructor or handling null path explicitly.
```suggestion
uriNoQueryParams = new URI(
uriWithParams.getScheme(),
uriWithParams.getUserInfo(),
uriWithParams.getHost(),
uriWithParams.getPort(),
uriWithParams.getPath() == null ? "" : uriWithParams.getPath(),
null,
null
);
```
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -120,68 +119,6 @@ public abstract class CloudSolrClient extends SolrClient {
protected volatile Object[] locks = objectList(3);
Review Comment:
Removal of the public CloudSolrClient.Builder class is a breaking API
change; consider retaining a deprecated shim delegating to
CloudHttp2SolrClient.Builder or clearly marking this removal in migration
documentation to prevent unexpected compile-time failures for existing client
code.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -374,7 +386,7 @@ public Builder withCollectionCacheTtl(long timeToLive,
TimeUnit unit) {
*
* @return this
*/
- public Builder withHttpClient(Http2SolrClient httpSolrClient) {
+ public Builder withHttpClient(HttpSolrClientBase httpSolrClient) {
if (this.internalClientBuilder != null) {
Review Comment:
Javadoc above still refers implicitly to the previous
Http2SolrClient-specific type; update the description to reflect the
generalized HttpSolrClientBase parameter to avoid confusion for API consumers.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -391,7 +403,7 @@ public Builder withHttpClient(Http2SolrClient
httpSolrClient) {
* @param internalClientBuilder the builder to use for creating the
internal http client.
* @return this
*/
- public Builder withHttpClientBuilder(Http2SolrClient.Builder
internalClientBuilder) {
+ public Builder withHttpClientBuilder(HttpSolrClientBuilderBase<?, ?>
internalClientBuilder) {
if (this.httpClient != null) {
Review Comment:
Javadoc should clarify acceptable builder types now that it accepts any
HttpSolrClientBuilderBase (e.g., Jetty or JDK variants); explicitly noting
expected implementations will improve usability.
--
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]
