You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "jdyer1 (via GitHub)" <gi...@apache.org> on 2025/10/03 22:37:37 UTC
[PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
jdyer1 opened a new pull request, #3730:
URL: https://github.com/apache/solr/pull/3730
This makes the necessary changes to `Http2ClusterStateProvider` to allow it to work with any subclass of `HttpSolrClientBase`, namely `HttpJdkSolrClient`. This required one behavior change: callers must pass an instance of `HttpSolrClientBase` to the constructor. Passing `null` will result in an `IllegalArgumentException`. (Internally, only the unit test was relying on prior behavior)
In addition method `withHttpClient` was added to `HttpJdkSolrClient.Builder`, allowing H2CSP to clone client instances with different base urls. Also a `builder()` method was added to the `HttpSolrClientBase` api so that H2CSP can obtain a builder of the correct type.
The unit test randomly chooses to test either client implementation.
In lieu of this approach, I considered creating a new class for the JDK client, leaving H2CSP unchanged. It seems better to make these value-add clients generic rather than creating yet more class variants.
This PR is targeted for main/10.x only.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3730:
URL: https://github.com/apache/solr/pull/3730#issuecomment-3371489824
> I am puzzled why we are getting this from precommit
Eric changed the hashes recently. A merge from main should fix.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 commented on PR #3730:
URL: https://github.com/apache/solr/pull/3730#issuecomment-3367546578
I am puzzled why we are getting this from `precommit`
```
Dependency checksum mismatch ('com.nvidia.cuvs.lucene:cuvs-lucene:25.10.0'), expected it to be: b5a458458255bfdfa6688a571b5bee032428f2d4, but was: 28c49fd9f03aa25dccc75da517247c9bc46e64b8
Dependency checksum mismatch ('com.nvidia.cuvs:cuvs-java:25.10.0'), expected it to be: 61f3a3ce565a659d296775a7b06fc20dabccee41, but was: 6c22acfbdbc7f3a4a78a2edea46124df872e9ea5
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 commented on code in PR #3730:
URL: https://github.com/apache/solr/pull/3730#discussion_r2407051587
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -47,6 +47,37 @@ public abstract class HttpSolrClientBuilderBase<
public abstract C build();
+ /**
+ * Provide a seed HttpSolrClient for the builder values, values can still be overridden by using
+ * builder methods
Review Comment:
I would have liked to make this a constructor-only option, but this is not new code and not a new doc-comment. I merely moved this for this PR/ticket, and would like to remain as focused as possible.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java:
##########
@@ -19,33 +19,34 @@
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) {
Review Comment:
I know some JDK code throws NPE in this situation, but it seems to me `IllegalArgumentException` is the better choice
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java:
##########
@@ -19,33 +19,34 @@
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 {
Review Comment:
Thank you for pointing this out. I am changing this to not close the client as this is what we did before in the case one was passed-in (indeed, our code always passes one in, so this would have been a regression I think).
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3730:
URL: https://github.com/apache/solr/pull/3730#discussion_r2403662347
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java:
##########
@@ -388,4 +388,11 @@ public ResponseParser getParser() {
public Set<String> getUrlParamNames() {
return urlParamNames;
}
+
+ /**
+ * Obtain a Builder that can be used to create new clients based on the setting of this client.
+ *
+ * @return a Builder
Review Comment:
just drop this ;-)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -47,6 +47,37 @@ public abstract class HttpSolrClientBuilderBase<
public abstract C build();
+ /**
+ * Provide a seed HttpSolrClient for the builder values, values can still be overridden by using
+ * builder methods
Review Comment:
Should document that any state of the current builder that has been set will not be changed.
Although... it'd be intuitive & simpler for this to work the reverse IMO. Basically, withHttpClient would set whatever state it sets no matter if withWhatever has been called already.
Even simpler and less error-prone would be to only have this be a constructor option.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java:
##########
@@ -388,4 +388,11 @@ public ResponseParser getParser() {
public Set<String> getUrlParamNames() {
return urlParamNames;
}
+
+ /**
+ * Obtain a Builder that can be used to create new clients based on the setting of this client.
+ *
+ * @return a Builder
+ */
+ public abstract HttpSolrClientBuilderBase<?, ?> builder();
Review Comment:
+1.
I suggest placing this below the constructors above rather than at the very bottom.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java:
##########
@@ -19,33 +19,34 @@
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 {
Review Comment:
Should document that we'll always close the client. Arguably we shouldn't if the caller isn't providing it, as that's the general rule of responsibility for closing things (creator closes).
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java:
##########
@@ -19,33 +19,34 @@
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) {
Review Comment:
Up to you but consider the conciseness of using the same line that sets httpClient to use `Object.requireNonNull(httpClient,"httpClient is required")`. True that it's an NPE and not IAE but...
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17943: ClusterStateProvider to support HttpJdkSolrClient [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 merged PR #3730:
URL: https://github.com/apache/solr/pull/3730
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org