You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "iamsanjay (via GitHub)" <gi...@apache.org> on 2024/11/01 05:46:55 UTC
[PR] SOLR-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
iamsanjay opened a new pull request, #2832:
URL: https://github.com/apache/solr/pull/2832
https://issues.apache.org/jira/browse/SOLR-17538
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2832:
URL: https://github.com/apache/solr/pull/2832#issuecomment-2457334718
CHANGES.txt: I would lean to not do so but it's borderline to me.
--
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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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-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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2832:
URL: https://github.com/apache/solr/pull/2832#discussion_r1827994098
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -233,6 +240,11 @@ public Builder(List<String> zkHosts, Optional<String> zkChroot) {
if (zkChroot.isPresent()) this.zkChroot = zkChroot.get();
}
+ /* for an expert use-case */
Review Comment:
Missing another asterisk
--
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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on code in PR #2832:
URL: https://github.com/apache/solr/pull/2832#discussion_r1826153383
##########
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:
Yes, actually it can be moved there. Totally make sense there. I guess we can move the whole factory logic along with this in builder.
--
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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on PR #2832:
URL: https://github.com/apache/solr/pull/2832#issuecomment-2457186134
@dsmiley Should there be change.txt for this?
--
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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay merged PR #2832:
URL: https://github.com/apache/solr/pull/2832
--
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-17538: CloudHttp2SolrClient needs a custom ClusterStateProvider option [solr]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on code in PR #2832:
URL: https://github.com/apache/solr/pull/2832#discussion_r1826151010
##########
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:
There were already two Factory method there -- one for ZkClientClusterStateProvider and another HttpCSP. My idea is to encapsulate that and the new custom logic into one unit. So that it should be easier for others to read this code. Yes I can move this class to builder so that we deal with stateProvider in the builder only rather than moving it to the main class.
--
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