Copilot commented on code in PR #3730:
URL: https://github.com/apache/solr/pull/3730#discussion_r2403510720
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -237,15 +290,10 @@ public void testClusterStateProviderOldVersion() throws
SolrServerException, IOE
}
try (var cspZk = zkClientClusterStateProvider();
- var cspHttp = http2ClusterStateProvider()) {
- // Even older SolrJ versionsg for non streamed response
- cspHttp
- .getHttpClient()
- .getHttpClient()
- .setUserAgentField(
- new HttpField(
- HttpHeader.USER_AGENT,
- "Solr[" + MethodHandles.lookup().lookupClass().getName() +
"] " + "2.0"));
+ // Even older SolrJ versionsg for non streamed response
Review Comment:
Corrected spelling of 'versionsg' to 'versions'.
```suggestion
// Even older SolrJ versions for non streamed response
```
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java:
##########
@@ -19,33 +19,32 @@
import java.io.IOException;
import java.util.List;
-import org.apache.solr.client.solrj.SolrClient;
-public class Http2ClusterStateProvider extends BaseHttpClusterStateProvider {
- final Http2SolrClient httpClient;
- final boolean closeClient;
+public class Http2ClusterStateProvider<C extends HttpSolrClientBase>
+ extends BaseHttpClusterStateProvider {
+ final C httpClient;
- public Http2ClusterStateProvider(List<String> solrUrls, Http2SolrClient
httpClient)
- throws Exception {
- this.httpClient = httpClient == null ? new
Http2SolrClient.Builder().build() : httpClient;
- this.closeClient = httpClient == null;
+ public Http2ClusterStateProvider(List<String> solrUrls, C httpClient) throws
Exception {
+ if (httpClient == null) {
+ throw new IllegalArgumentException("You must provide an Http client.");
+ }
+ this.httpClient = httpClient;
initConfiguredNodes(solrUrls);
}
@Override
public void close() throws IOException {
- if (this.closeClient && this.httpClient != null) {
- httpClient.close();
- }
+ httpClient.close();
Review Comment:
Potential null pointer dereference. While the constructor now validates that
httpClient is not null, this method should still guard against null to be
defensive.
```suggestion
if (httpClient != null) {
httpClient.close();
}
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -76,18 +100,52 @@ public static Iterable<String[]> parameters() throws
NoSuchMethodException {
new String[] {"http2ClusterStateProvider"}, new String[]
{"zkClientClusterStateProvider"});
}
- private static Http2ClusterStateProvider http2ClusterStateProvider() {
+ private static Http2ClusterStateProvider<?> http2ClusterStateProvider(String
userAgent) {
try {
- return new Http2ClusterStateProvider(
- List.of(
- cluster.getJettySolrRunner(0).getBaseUrl().toString(),
- cluster.getJettySolrRunner(1).getBaseUrl().toString()),
- null);
+ var useJdkProvider = random().nextBoolean();
+ HttpSolrClientBase client;
+
+ if (userAgent != null) {
+ if (useJdkProvider) {
+ client =
+ new UserAgentChangingJdkClient(
+ new HttpJdkSolrClient.Builder()
+
.withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT),
+ userAgent);
+ } else {
+ var http2SolrClient = new Http2SolrClient.Builder().build();
+ http2SolrClient
+ .getHttpClient()
+ .setUserAgentField(new HttpField(HttpHeader.USER_AGENT,
userAgent));
+ client = http2SolrClient;
+ }
+ } else {
+ client =
+ useJdkProvider
+ ? new HttpJdkSolrClient.Builder()
+ .withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT)
+ .build()
+ : new Http2SolrClient.Builder().build();
+ }
+ var clientClassName = client.getClass().getName();
+ log.info("Using Http client implementaton: {}", clientClassName);
Review Comment:
Corrected spelling of 'implementaton' to 'implementation'.
```suggestion
log.info("Using Http client implementation: {}", clientClassName);
```
--
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]
