Copilot commented on code in PR #3774:
URL: https://github.com/apache/solr/pull/3774#discussion_r2433693375
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -136,14 +175,20 @@ public static void tearDownAfterClass() throws Exception {
}
shutdownCluster();
- httpBasedCloudSolrClient = null;
+ httpJettyBasedCloudSolrClient = null;
zkBasedCloudSolrClient = null;
}
/** Randomly return the cluster's ZK based CSC, or HttpClusterProvider based
CSC. */
private CloudSolrClient getRandomClient() {
- // return random().nextBoolean()? zkBasedCloudSolrClient :
httpBasedCloudSolrClient;
- return httpBasedCloudSolrClient;
+ int randInt = random().nextInt(2);
+ if (randInt == 0) {
+ return zkBasedCloudSolrClient;
+ }
+ if (randInt == 1) {
+ return httpJettyBasedCloudSolrClient;
+ }
+ return httpJdkBasedCloudSolrClient;
Review Comment:
The branch returning httpJdkBasedCloudSolrClient is unreachable because
nextInt(2) only yields 0 or 1; the JDK-based client will never be returned. Use
random().nextInt(3) or a different selection mechanism (e.g., List + random
index) to include all three clients.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -136,14 +175,20 @@ public static void tearDownAfterClass() throws Exception {
}
shutdownCluster();
- httpBasedCloudSolrClient = null;
+ httpJettyBasedCloudSolrClient = null;
Review Comment:
[nitpick] httpJdkBasedCloudSolrClient is closed earlier but not nulled here
like the other clients; nulling it for consistency avoids retaining a stale
static reference.
```suggestion
httpJettyBasedCloudSolrClient = null;
httpJdkBasedCloudSolrClient = null;
```
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -379,24 +380,25 @@ 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);
} catch (URISyntaxException e) {
// If the url is invalid, let a subsequent request try again.
return false;
}
+ return maybeTryHeadRequestSync(uriNoQueryParams);
Review Comment:
The caching key uses the full URI including query parameters, causing
multiple HEAD requests for the same base endpoint when query strings differ;
construct a normalized URI without query/fragment (e.g., new URI(u.getScheme(),
u.getAuthority(), u.getPath(), null, null)) before caching.
--
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]
