dsmiley commented on code in PR #2832:
URL: https://github.com/apache/solr/pull/2832#discussion_r1826017473
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -177,6 +177,10 @@ public Builder(List<String> solrUrls) {
public Builder(List<String> zkHosts, Optional<String> zkChroot) {
super(zkHosts, zkChroot);
}
+
Review Comment:
add a line of javadoc saying this is for expert use-cases.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -233,6 +256,10 @@ public Builder(List<String> zkHosts, Optional<String>
zkChroot) {
if (zkChroot.isPresent()) this.zkChroot = zkChroot.get();
}
+ public Builder(ClusterStateProvider stateProvider) {
Review Comment:
It should have javadoc; in this case saying this is for an expert use-case
(needn't say more).
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -56,10 +56,14 @@ protected CloudHttp2SolrClient(Builder builder) {
super(builder.shardLeadersOnly, builder.parallelUpdates,
builder.directUpdatesToLeadersOnly);
this.clientIsInternal = builder.httpClient == null;
this.myClient = createOrGetHttpClientFromBuilder(builder);
- this.stateProvider =
- builder.zkHosts.isEmpty()
- ? createHttp2ClusterStateProvider(builder.solrUrls, myClient)
- : createZkClusterStateProvider(builder);
+
+ try {
+ ClusterStateProviderFactory factory = new
ClusterStateProviderFactory(builder, myClient);
+ this.stateProvider = factory.create();
Review Comment:
Looking at this; I have to wonder if we really need a Factory class at all
or do we just need a factory *method*. Maybe on ClusterStateProvider or the
builder. Factory classes are useful if it's pluggable or configurable but
that's not the case here.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -451,12 +478,19 @@ public Builder withZkClientTimeout(int zkClientTimeout,
TimeUnit unit) {
/** Create a {@link CloudHttp2SolrClient} based on the provided
configuration. */
public CloudHttp2SolrClient build() {
- if (!zkHosts.isEmpty() && !solrUrls.isEmpty()) {
+ int providedOptions = 0;
+ if (!zkHosts.isEmpty()) providedOptions++;
+ if (!solrUrls.isEmpty()) providedOptions++;
+ if (stateProvider != null) providedOptions++;
+
+ if (providedOptions > 1) {
throw new IllegalArgumentException(
- "Both zkHost(s) & solrUrl(s) have been specified. Only specify
one.");
- } else if (zkHosts.isEmpty() && solrUrls.isEmpty()) {
- throw new IllegalArgumentException("Both zkHosts and solrUrl cannot be
null.");
+ "Only one of zkHost(s), solrUrl(s), or stateProvider should be
specified.");
+ } else if (providedOptions == 0) {
+ throw new IllegalArgumentException(
+ "One of zkHosts, solrUrls, or stateProvider must be specified.");
}
Review Comment:
As you are separating out a Factory for the ClusterStateProvider, I wonder
if this code here might move over there as it's tightly correlated with it?
Not important; just a thought.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
