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: [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]
