You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/02/25 12:54:56 UTC
[GitHub] [solr] haythemkh opened a new pull request #708: SOLR-15342: Separate out a SolrJ-Zookeeper module
haythemkh opened a new pull request #708:
URL: https://github.com/apache/solr/pull/708
https://issues.apache.org/jira/browse/SOLR-15342
# Description
Extraction of BaseCloudSolrClient public methods as a first step in the **solrj** module split work.
# Tests
Used `./gradlew test` and `./gradlew check` to check tests are still green. I'm still working on fixing some failing tests. An example of test failure:
> java.lang.NullPointerException
> at __randomizedtesting.SeedInfo.seed([E7AED408A8599C5:862ED29A2479F43D]:0)
> at org.apache.solr.common.cloud.ZkStateReader.from(ZkStateReader.java:224)
> at org.apache.solr.cloud.AbstractFullDistribZkTestBase.waitForActiveReplicaCount(AbstractFullDistribZkTestBase.java:592)
> at org.apache.solr.cloud.AbstractFullDistribZkTestBase.createServers(AbstractFullDistribZkTestBase.java:333)
> at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsFixedStatement.callStatement(BaseDistributedSearchTestCase.java:1079)
> at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsStatement.evaluate(BaseDistributedSearchTestCase.java:1052)
> at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:57)
> at org.junit.rules.RunRules.evaluate(RunRules.java:20)
> at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:44)
> at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
> at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
> at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
> at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
> at org.junit.rules.RunRules.evaluate(RunRules.java:20)
> at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
> at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:370)
> at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:819)
> at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:470)
> at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:951)
> at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:836)
> at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:887)
> at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:898)
> at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
> at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
> at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
> at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:57)
> at org.junit.rules.RunRules.evaluate(RunRules.java:20)
> at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
> at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
> at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:38)
> at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
> at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
> at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
> at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
> at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
> at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
> at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
> at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
> at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:47)
> at org.junit.rules.RunRules.evaluate(RunRules.java:20)
> at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
> at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:370)
> at com.carrotsearch.randomizedtesting.ThreadLeakControl.lambda$forkTimeoutingTask$0(ThreadLeakControl.java:826)
> at java.base/java.lang.Thread.run(Thread.java:834)`
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] 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)
- [ ] I have developed this patch against the `main` branch.
- [ ] 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
[GitHub] [solr] HoustonPutman commented on pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #708:
URL: https://github.com/apache/solr/pull/708#issuecomment-1060901299
Agreed that this is a good step. We definitely should include upgrade notes in the ref-guide, as david mentioned. Also needs a changes.txt and a `./gradlew spotlessApply` for good measure.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825355477
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
##########
@@ -358,4 +358,12 @@ public void setCacheTimeout(int cacheTimeout) {
// This exception is not meant to escape this class it should be caught and wrapped.
private class NotACollectionException extends Exception {}
+
+ @Override
+ public String getQuorumHosts() {
+ if (this.liveNodes == null) {
+ return null;
+ }
+ return String.join(",", this.liveNodes);
Review comment:
Who knows :-). This info, I think, is more for informational purposes than having a solid definition at an abstract level. If you wish to propose something different, please do!
--
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
[GitHub] [solr] janhoy commented on pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #708:
URL: https://github.com/apache/solr/pull/708#issuecomment-1065510681
There are a bunch of test failures in `org.apache.solr.cloud.OverseerCollectionConfigSetProcessorTest` - NPE, did not have time to dive in
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825347816
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
##########
@@ -358,4 +358,12 @@ public void setCacheTimeout(int cacheTimeout) {
// This exception is not meant to escape this class it should be caught and wrapped.
private class NotACollectionException extends Exception {}
+
+ @Override
+ public String getQuorumHosts() {
+ if (this.liveNodes == null) {
+ return null;
+ }
+ return String.join(",", this.liveNodes);
Review comment:
Ah, so quorum here is either a list of zk hosts or a list of Solr hosts. I get it. But what if Someone implements a state provider that gets state from a Unix socket or something that is not hostname based?
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r824374698
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
##########
@@ -221,6 +223,19 @@
// only kept to identify if the cleaner has already been started.
private Future<?> collectionPropsCacheCleaner;
+ private static BaseCloudSolrClient client;
Review comment:
Important: this addition is definitely not cool. The refactoring here shouldn't be adding state (fields on classes) to anything.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r824864772
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java
##########
@@ -215,6 +215,11 @@ static String buildZkHostString(Collection<String> zkHosts, String chroot) {
return zkHostString;
}
+ @Override
+ public String getQuorumHosts() {
+ return zkStateReader.getZkClient().getZkServerAddress();
Review comment:
Even when I replace `zkStateReader` with `getZkStateReader()`, some tests fail.
I believe the `CloudHttp2SolrClientBuilderTest` was designed to simply test that the (dummy) ZK_HOST provided is being set, but the way the test was changed in this PR makes it want to connect to that ZK, which of course fails...
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r824843165
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java
##########
@@ -215,6 +215,11 @@ static String buildZkHostString(Collection<String> zkHosts, String chroot) {
return zkHostString;
}
+ @Override
+ public String getQuorumHosts() {
+ return zkStateReader.getZkClient().getZkServerAddress();
Review comment:
This seems to hit a NPE in a bunch of the recent test failures.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825347146
##########
File path: solr/core/src/test/org/apache/solr/search/join/CrossCollectionJoinQueryTest.java
##########
@@ -197,7 +197,7 @@ public void testCcJoinRoutedCollection() throws Exception {
String.format(
Locale.ROOT,
"{!join method=crossCollection zkHost=\"%s\" fromIndex=products from=product_id_s to=product_id_s}size_s:M",
- cluster.getSolrClient().getZkHost()),
+ client.getClusterStateProvider().getQuorumHosts()),
Review comment:
I thought this test failed with NPE, but if it passes it’s probably ok.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r824973440
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
##########
@@ -221,6 +223,16 @@
// only kept to identify if the cleaner has already been started.
private Future<?> collectionPropsCacheCleaner;
+ public static ZkStateReader from(BaseCloudSolrClient solrClient) {
Review comment:
Please add Javadoc
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r814906422
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrSchema.java
##########
@@ -88,7 +88,7 @@ public boolean isClosed() {
protected Map<String, Table> getTableMap() {
String zk = this.properties.getProperty("zk");
CloudSolrClient cloudSolrClient = solrClientCache.getCloudSolrClient(zk);
- ZkStateReader zkStateReader = cloudSolrClient.getZkStateReader();
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudSolrClient);
Review comment:
I said I wouldn't repeat that comment I just wrote but I observe here a basic/trivial "find" wouldn't find this case. From the GitHub UI it's not clear is zkStateReader is in fact still needed; perhaps it is.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r814891991
##########
File path: solr/core/src/test/org/apache/solr/handler/TestBlobHandler.java
##########
@@ -65,7 +65,7 @@ public void doBlobHandlerTest() throws Exception {
response1 = createCollectionRequest.process(client);
assertEquals(0, response1.getStatus());
assertTrue(response1.isSuccess());
- DocCollection sysColl = ZkStateReader.from(client).getClusterState().getCollection(".system");
+ DocCollection sysColl = ZkStateReader.from(cloudClient).getClusterState().getCollection(".system");
Review comment:
ZkStateReader isn't needed
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/CloudSolrClientUtils.java
##########
@@ -0,0 +1,134 @@
+package org.apache.solr.cloud;
+
+import org.apache.solr.client.solrj.impl.BaseCloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.common.cloud.*;
+
+import java.io.Serializable;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+
+public class CloudSolrClientUtils implements Serializable {
+ /**
+ * @return the zkHost value used to connect to zookeeper.
+ */
+ public static String getZkHost(BaseCloudSolrClient client) {
+ return assertZKStateProvider(client).zkHost;
+ }
+
+ /**
+ * Set the connect timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkConnectTimeout(BaseCloudSolrClient client, int zkConnectTimeout) {
+ assertZKStateProvider(client).zkConnectTimeout = zkConnectTimeout;
+ }
+
+ /**
+ * Set the timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkClientTimeout(BaseCloudSolrClient client, int zkClientTimeout) {
+ assertZKStateProvider(client).zkClientTimeout = zkClientTimeout;
+ }
+
+ static ZkClientClusterStateProvider assertZKStateProvider(BaseCloudSolrClient client) {
+ if (client.getClusterStateProvider() instanceof ZkClientClusterStateProvider) {
+ return (ZkClientClusterStateProvider) client.getClusterStateProvider();
+ }
+ throw new IllegalArgumentException("This client does not use ZK");
+
+ }
+
+ /**
+ * Block until a CollectionStatePredicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * <p>
+ * This implementation utilizes {@link CollectionStateWatcher} internally.
+ * Callers that don't care about liveNodes are encouraged to use a {@link DocCollection} {@link Predicate}
+ * instead
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link CollectionStatePredicate} to check the collection state
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #waitForState(BaseCloudSolrClient, String, long, TimeUnit, Predicate)
+ * @see #registerCollectionStateWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, CollectionStatePredicate predicate)
+ throws InterruptedException, TimeoutException {
+ client.getClusterStateProvider().connect();
+ assertZKStateProvider(client).getZkStateReader().waitForState(collection, wait, unit, predicate);
+ }
+
+ /**
+ * Block until a Predicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link Predicate} to test against the {@link DocCollection}
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #registerDocCollectionWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, Predicate<DocCollection> predicate)
Review comment:
same as I said above
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/CloudSolrClientUtils.java
##########
@@ -0,0 +1,134 @@
+package org.apache.solr.cloud;
+
+import org.apache.solr.client.solrj.impl.BaseCloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.common.cloud.*;
+
+import java.io.Serializable;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+
+public class CloudSolrClientUtils implements Serializable {
+ /**
+ * @return the zkHost value used to connect to zookeeper.
+ */
+ public static String getZkHost(BaseCloudSolrClient client) {
+ return assertZKStateProvider(client).zkHost;
+ }
+
+ /**
+ * Set the connect timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkConnectTimeout(BaseCloudSolrClient client, int zkConnectTimeout) {
+ assertZKStateProvider(client).zkConnectTimeout = zkConnectTimeout;
+ }
+
+ /**
+ * Set the timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkClientTimeout(BaseCloudSolrClient client, int zkClientTimeout) {
+ assertZKStateProvider(client).zkClientTimeout = zkClientTimeout;
+ }
+
+ static ZkClientClusterStateProvider assertZKStateProvider(BaseCloudSolrClient client) {
+ if (client.getClusterStateProvider() instanceof ZkClientClusterStateProvider) {
+ return (ZkClientClusterStateProvider) client.getClusterStateProvider();
+ }
+ throw new IllegalArgumentException("This client does not use ZK");
+
+ }
+
+ /**
+ * Block until a CollectionStatePredicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * <p>
+ * This implementation utilizes {@link CollectionStateWatcher} internally.
+ * Callers that don't care about liveNodes are encouraged to use a {@link DocCollection} {@link Predicate}
+ * instead
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link CollectionStatePredicate} to check the collection state
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #waitForState(BaseCloudSolrClient, String, long, TimeUnit, Predicate)
+ * @see #registerCollectionStateWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, CollectionStatePredicate predicate)
Review comment:
Instead, I think we should make the caller call ZkStateReader.from(client).waitForState
If you change the method implementation here to do that, you can then tell IntelliJ to *inline* this method, and it'll do all the work for you.
Ensure waitForState over there has this same great javadocs.
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/CloudSolrClientUtils.java
##########
@@ -0,0 +1,134 @@
+package org.apache.solr.cloud;
+
+import org.apache.solr.client.solrj.impl.BaseCloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.common.cloud.*;
+
+import java.io.Serializable;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+
+public class CloudSolrClientUtils implements Serializable {
+ /**
+ * @return the zkHost value used to connect to zookeeper.
+ */
+ public static String getZkHost(BaseCloudSolrClient client) {
+ return assertZKStateProvider(client).zkHost;
+ }
+
+ /**
+ * Set the connect timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkConnectTimeout(BaseCloudSolrClient client, int zkConnectTimeout) {
+ assertZKStateProvider(client).zkConnectTimeout = zkConnectTimeout;
+ }
+
+ /**
+ * Set the timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkClientTimeout(BaseCloudSolrClient client, int zkClientTimeout) {
+ assertZKStateProvider(client).zkClientTimeout = zkClientTimeout;
+ }
+
+ static ZkClientClusterStateProvider assertZKStateProvider(BaseCloudSolrClient client) {
+ if (client.getClusterStateProvider() instanceof ZkClientClusterStateProvider) {
+ return (ZkClientClusterStateProvider) client.getClusterStateProvider();
+ }
+ throw new IllegalArgumentException("This client does not use ZK");
+
+ }
+
+ /**
+ * Block until a CollectionStatePredicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * <p>
+ * This implementation utilizes {@link CollectionStateWatcher} internally.
+ * Callers that don't care about liveNodes are encouraged to use a {@link DocCollection} {@link Predicate}
+ * instead
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link CollectionStatePredicate} to check the collection state
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #waitForState(BaseCloudSolrClient, String, long, TimeUnit, Predicate)
+ * @see #registerCollectionStateWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, CollectionStatePredicate predicate)
+ throws InterruptedException, TimeoutException {
+ client.getClusterStateProvider().connect();
+ assertZKStateProvider(client).getZkStateReader().waitForState(collection, wait, unit, predicate);
+ }
+
+ /**
+ * Block until a Predicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link Predicate} to test against the {@link DocCollection}
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #registerDocCollectionWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, Predicate<DocCollection> predicate)
+ throws InterruptedException, TimeoutException {
+ client.getClusterStateProvider().connect();
+ assertZKStateProvider(client).getZkStateReader().waitForState(collection, wait, unit, predicate);
+ }
+
+ /**
+ * Register a CollectionStateWatcher to be called when the cluster state for a collection changes
+ * <em>or</em> the set of live nodes changes.
+ *
+ * <p>
+ * The Watcher will automatically be removed when it's
+ * <code>onStateChanged</code> returns <code>true</code>
+ * </p>
+ *
+ * <p>
+ * This implementation utilizes {@link ZkStateReader#registerCollectionStateWatcher} internally.
+ * Callers that don't care about liveNodes are encouraged to use a {@link DocCollectionWatcher}
+ * instead
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param watcher a watcher that will be called when the state changes
+ * @see #registerDocCollectionWatcher(BaseCloudSolrClient, String, DocCollectionWatcher)
+ * @see ZkStateReader#registerCollectionStateWatcher
+ */
+ public static void registerCollectionStateWatcher(BaseCloudSolrClient client, String collection, CollectionStateWatcher watcher) {
+ client.getClusterStateProvider().connect();
+ assertZKStateProvider(client).getZkStateReader().registerCollectionStateWatcher(collection, watcher);
+ }
+
+ /**
+ * Register a DocCollectionWatcher to be called when the cluster state for a collection changes.
+ *
+ * <p>
+ * The Watcher will automatically be removed when it's
+ * <code>onStateChanged</code> returns <code>true</code>
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param watcher a watcher that will be called when the state changes
+ * @see ZkStateReader#registerDocCollectionWatcher
+ */
+ public static void registerDocCollectionWatcher(BaseCloudSolrClient client, String collection, DocCollectionWatcher watcher) {
Review comment:
same as I said above
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/CloudSolrClientUtils.java
##########
@@ -0,0 +1,134 @@
+package org.apache.solr.cloud;
+
+import org.apache.solr.client.solrj.impl.BaseCloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.common.cloud.*;
+
+import java.io.Serializable;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+
+public class CloudSolrClientUtils implements Serializable {
+ /**
+ * @return the zkHost value used to connect to zookeeper.
+ */
+ public static String getZkHost(BaseCloudSolrClient client) {
+ return assertZKStateProvider(client).zkHost;
+ }
+
+ /**
+ * Set the connect timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkConnectTimeout(BaseCloudSolrClient client, int zkConnectTimeout) {
+ assertZKStateProvider(client).zkConnectTimeout = zkConnectTimeout;
+ }
+
+ /**
+ * Set the timeout to the zookeeper ensemble in ms
+ */
+ public static void setZkClientTimeout(BaseCloudSolrClient client, int zkClientTimeout) {
+ assertZKStateProvider(client).zkClientTimeout = zkClientTimeout;
+ }
+
+ static ZkClientClusterStateProvider assertZKStateProvider(BaseCloudSolrClient client) {
+ if (client.getClusterStateProvider() instanceof ZkClientClusterStateProvider) {
+ return (ZkClientClusterStateProvider) client.getClusterStateProvider();
+ }
+ throw new IllegalArgumentException("This client does not use ZK");
+
+ }
+
+ /**
+ * Block until a CollectionStatePredicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * <p>
+ * This implementation utilizes {@link CollectionStateWatcher} internally.
+ * Callers that don't care about liveNodes are encouraged to use a {@link DocCollection} {@link Predicate}
+ * instead
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link CollectionStatePredicate} to check the collection state
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #waitForState(BaseCloudSolrClient, String, long, TimeUnit, Predicate)
+ * @see #registerCollectionStateWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, CollectionStatePredicate predicate)
+ throws InterruptedException, TimeoutException {
+ client.getClusterStateProvider().connect();
+ assertZKStateProvider(client).getZkStateReader().waitForState(collection, wait, unit, predicate);
+ }
+
+ /**
+ * Block until a Predicate returns true, or the wait times out
+ *
+ * <p>
+ * Note that the predicate may be called again even after it has returned true, so
+ * implementors should avoid changing state within the predicate call itself.
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param wait how long to wait
+ * @param unit the units of the wait parameter
+ * @param predicate a {@link Predicate} to test against the {@link DocCollection}
+ * @throws InterruptedException on interrupt
+ * @throws TimeoutException on timeout
+ * @see #registerDocCollectionWatcher
+ */
+ public static void waitForState(BaseCloudSolrClient client, String collection, long wait, TimeUnit unit, Predicate<DocCollection> predicate)
+ throws InterruptedException, TimeoutException {
+ client.getClusterStateProvider().connect();
+ assertZKStateProvider(client).getZkStateReader().waitForState(collection, wait, unit, predicate);
+ }
+
+ /**
+ * Register a CollectionStateWatcher to be called when the cluster state for a collection changes
+ * <em>or</em> the set of live nodes changes.
+ *
+ * <p>
+ * The Watcher will automatically be removed when it's
+ * <code>onStateChanged</code> returns <code>true</code>
+ * </p>
+ *
+ * <p>
+ * This implementation utilizes {@link ZkStateReader#registerCollectionStateWatcher} internally.
+ * Callers that don't care about liveNodes are encouraged to use a {@link DocCollectionWatcher}
+ * instead
+ * </p>
+ *
+ * @param collection the collection to watch
+ * @param watcher a watcher that will be called when the state changes
+ * @see #registerDocCollectionWatcher(BaseCloudSolrClient, String, DocCollectionWatcher)
+ * @see ZkStateReader#registerCollectionStateWatcher
+ */
+ public static void registerCollectionStateWatcher(BaseCloudSolrClient client, String collection, CollectionStateWatcher watcher) {
Review comment:
same as I said above
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825252043
##########
File path: solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java
##########
@@ -160,7 +160,7 @@ protected MetricSamples request(SolrClient client, MetricsQuery query) throws IO
if (client instanceof CloudSolrClient) {
labelNames.add("zk_host");
- labelValues.add(((CloudSolrClient) client).getZkHost());
+ labelValues.add(((CloudSolrClient) client).getClusterStateProvider().getQuorumHosts());
Review comment:
Again, this is intentional to be generic
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825252446
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java
##########
@@ -100,4 +100,6 @@ default Object getClusterProperty(String propertyName) {
String getPolicyNameByCollection(String coll);
void connect();
+
+ String getQuorumHosts();
Review comment:
This was my idea to avoid having BaseCloudSolrClient.getZkHosts which is a no-no now.
--
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
[GitHub] [solr] janhoy edited a comment on pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy edited a comment on pull request #708:
URL: https://github.com/apache/solr/pull/708#issuecomment-1065399440
And the SolrJ tests are GREEN! Still a bunch of other tests fail though.
I'll bring it up to date with latest main and then some last round of peer review...
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825252854
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java
##########
@@ -39,9 +39,9 @@
volatile ZkStateReader zkStateReader;
private boolean closeZkStateReader = true;
- String zkHost;
- int zkConnectTimeout = 15000;
- int zkClientTimeout = 45000;
+ public String zkHost;
+ public int zkConnectTimeout = 15000;
Review comment:
These were being manipulated by utility methods. Those methods used to be on BaseCloudSolrClient and were moved to a new class CloudSolrClientUtils. This is a bit icky; I don't like making fields public, nor the mutability of anything SolrClient related for that matter. @haythemkh shouldn't CloudSolrClientUtils be in the same package maybe?
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r824989316
##########
File path: solr/core/src/test/org/apache/solr/cloud/DeleteReplicaTest.java
##########
@@ -36,13 +36,7 @@
import org.apache.solr.cloud.overseer.OverseerAction;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.DocCollectionWatcher;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
-import org.apache.solr.common.cloud.ZkNodeProps;
-import org.apache.solr.common.cloud.ZkStateReader;
-import org.apache.solr.common.cloud.ZkStateReaderAccessor;
+import org.apache.solr.common.cloud.*;
Review comment:
Same comment here as above about wildcard imports
##########
File path: solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java
##########
@@ -38,10 +38,7 @@
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.cloud.SolrCloudManager;
import org.apache.solr.client.solrj.embedded.JettySolrRunner;
-import org.apache.solr.client.solrj.impl.CloudSolrClient;
-import org.apache.solr.client.solrj.impl.ClusterStateProvider;
-import org.apache.solr.client.solrj.impl.HttpSolrClient;
-import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.client.solrj.impl.*;
Review comment:
Think we encourage listing each import rather than `*`, you can change your IDE setting
##########
File path: solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java
##########
@@ -36,10 +36,7 @@
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.DocRouter;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.ZkCoreNodeProps;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard import
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudDeleteByQuery.java
##########
@@ -128,7 +128,7 @@ private static void createMiniSolrCloudCluster() throws Exception {
CLOUD_CLIENT = cluster.getSolrClient();
CLOUD_CLIENT.setDefaultCollection(COLLECTION_NAME);
- ZkStateReader zkStateReader = CLOUD_CLIENT.getZkStateReader();
+ ZkStateReader zkStateReader = ZkStateReader.from(cluster.getSolrClient());
Review comment:
```suggestion
ZkStateReader zkStateReader = ZkStateReader.from(CLOUD_CLIENT);
```
##########
File path: solr/core/src/test/org/apache/solr/search/join/CrossCollectionJoinQueryTest.java
##########
@@ -197,7 +197,7 @@ public void testCcJoinRoutedCollection() throws Exception {
String.format(
Locale.ROOT,
"{!join method=crossCollection zkHost=\"%s\" fromIndex=products from=product_id_s to=product_id_s}size_s:M",
- cluster.getSolrClient().getZkHost()),
+ client.getClusterStateProvider().getQuorumHosts()),
Review comment:
Think we need the same trick here with something like
```
((ZkClusterStateProvider)client.getClusterStateProvider()).zkHost),
```
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java
##########
@@ -100,4 +100,6 @@ default Object getClusterProperty(String propertyName) {
String getPolicyNameByCollection(String coll);
void connect();
+
+ String getQuorumHosts();
Review comment:
@haythemkh , @dsmiley Can you explain the main reason for this new method - I see that a lot of places which earlier just fetched the *configured* zk string, now calls this method, which for ZkClusterStateProvider requires an initialized zk connection...
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
##########
@@ -358,4 +358,12 @@ public void setCacheTimeout(int cacheTimeout) {
// This exception is not meant to escape this class it should be caught and wrapped.
private class NotACollectionException extends Exception {}
+
+ @Override
+ public String getQuorumHosts() {
+ if (this.liveNodes == null) {
+ return null;
+ }
+ return String.join(",", this.liveNodes);
Review comment:
Why is livenodes returned here? I thought QuorumHosts was the Zookeeper hosts, not the solr hosts?
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZkTestBase.java
##########
@@ -1505,9 +1509,10 @@ private void testANewCollectionInOneInstance() throws Exception {
SolrClient client3 = collectionClients.get(2);
SolrClient client4 = collectionClients.get(3);
- waitForRecoveriesToFinish(
- oneInstanceCollection, getCommonCloudSolrClient().getZkStateReader(), false);
- assertAllActive(oneInstanceCollection, getCommonCloudSolrClient().getZkStateReader());
+ getCommonCloudSolrClient();
+ waitForRecoveriesToFinish(oneInstanceCollection, ZkStateReader.from(cloudClient), false);
+ getCommonCloudSolrClient();
+ assertAllActive(oneInstanceCollection, ZkStateReader.from(cloudClient));
Review comment:
Can you explain why `getCommonCloudSolrClient()` is called twice here and why the return value is not used?
##########
File path: solr/core/src/test/org/apache/solr/cloud/MigrateRouteKeyTest.java
##########
@@ -29,10 +29,7 @@
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.RoutingRule;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard import...
##########
File path: solr/core/src/test/org/apache/solr/cloud/DistribDocExpirationUpdateProcessorTest.java
##########
@@ -36,9 +36,7 @@
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard import
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -249,8 +248,9 @@ private void addDocWhenOtherReplicasAreNetworkPartitioned(
TimeoutException.class,
"Did not time out waiting for new leader, out of sync replica became leader",
() -> {
- cluster
- .getSolrClient()
+ // this is is the bad case, our "bad" state was found before timeout
Review comment:
```suggestion
// this is the bad case, our "bad" state was found before timeout
```
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestLeaderElectionWithEmptyReplica.java
##########
@@ -28,9 +28,7 @@
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Get rid of wildcard import
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestTolerantUpdateProcessorCloud.java
##########
@@ -117,7 +117,7 @@ public static void createMiniSolrCloudCluster() throws Exception {
cluster.waitForActiveCollection(COLLECTION_NAME, NUM_SHARDS, REPLICATION_FACTOR * NUM_SHARDS);
- ZkStateReader zkStateReader = CLOUD_CLIENT.getZkStateReader();
+ ZkStateReader zkStateReader = ZkStateReader.from(cluster.getSolrClient());
Review comment:
```suggestion
ZkStateReader zkStateReader = ZkStateReader.from(CLOUD_CLIENT);
```
##########
File path: solr/core/src/test/org/apache/solr/update/DeleteByIdWithRouterFieldTest.java
##########
@@ -33,10 +33,7 @@
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrDocumentList;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.ClusterState;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard...
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
##########
@@ -64,17 +64,7 @@
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.ToleratedUpdateError;
-import org.apache.solr.common.cloud.ClusterState;
-import org.apache.solr.common.cloud.CollectionStatePredicate;
-import org.apache.solr.common.cloud.CollectionStateWatcher;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.DocCollectionWatcher;
-import org.apache.solr.common.cloud.DocRouter;
-import org.apache.solr.common.cloud.ImplicitDocRouter;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
-import org.apache.solr.common.cloud.ZkCoreNodeProps;
-import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.cloud.*;
Review comment:
Fix wildcard imports
##########
File path: solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java
##########
@@ -46,20 +46,9 @@
import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.client.solrj.response.RequestStatusState;
-import org.apache.solr.cloud.AbstractDistribZkTestBase;
-import org.apache.solr.cloud.BasicDistributedZkTest;
-import org.apache.solr.cloud.SolrCloudTestCase;
-import org.apache.solr.cloud.StoppableIndexingThread;
+import org.apache.solr.cloud.*;
Review comment:
Fix wildcard imports
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java
##########
@@ -39,9 +39,9 @@
volatile ZkStateReader zkStateReader;
private boolean closeZkStateReader = true;
- String zkHost;
- int zkConnectTimeout = 15000;
- int zkClientTimeout = 45000;
+ public String zkHost;
+ public int zkConnectTimeout = 15000;
Review comment:
Without having looked, why the change to public here?
##########
File path: solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java
##########
@@ -160,7 +160,7 @@ protected MetricSamples request(SolrClient client, MetricsQuery query) throws IO
if (client instanceof CloudSolrClient) {
labelNames.add("zk_host");
- labelValues.add(((CloudSolrClient) client).getZkHost());
+ labelValues.add(((CloudSolrClient) client).getClusterStateProvider().getQuorumHosts());
Review comment:
Should instead just get the zkHost string
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/DelegatingClusterStateProvider.java
##########
@@ -124,4 +124,12 @@ public void close() throws IOException {
delegate.close();
}
}
+
+ @Override
+ public String getQuorumHosts() {
+ if (delegate != null) {
+ return delegate.getQuorumHosts();
+ }
+ return null;
Review comment:
What does it mean to return null here? Should it instead throw exception?
--
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
[GitHub] [solr] dsmiley merged pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #708:
URL: https://github.com/apache/solr/pull/708
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825252017
##########
File path: solr/core/src/test/org/apache/solr/update/DeleteByIdWithRouterFieldTest.java
##########
@@ -33,10 +33,7 @@
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrDocumentList;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.ClusterState;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
yeah :-( Let's address this repo wide. I'll do it. It may not get done in this PR though.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825252089
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/DelegatingClusterStateProvider.java
##########
@@ -124,4 +124,12 @@ public void close() throws IOException {
delegate.close();
}
}
+
+ @Override
+ public String getQuorumHosts() {
+ if (delegate != null) {
+ return delegate.getQuorumHosts();
+ }
+ return null;
Review comment:
From the diff it may not be apparent but the behavior here mimicks the other methods
--
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
[GitHub] [solr] janhoy commented on pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #708:
URL: https://github.com/apache/solr/pull/708#issuecomment-1065374182
I pushed a commit that I believe fixes the failing tests...
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825252245
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
##########
@@ -358,4 +358,12 @@ public void setCacheTimeout(int cacheTimeout) {
// This exception is not meant to escape this class it should be caught and wrapped.
private class NotACollectionException extends Exception {}
+
+ @Override
+ public String getQuorumHosts() {
+ if (this.liveNodes == null) {
+ return null;
+ }
+ return String.join(",", this.liveNodes);
Review comment:
It's the set of servers we get the state from. For a Solr based provider, it's Solr not ZK.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825357518
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZkTestBase.java
##########
@@ -1505,9 +1509,10 @@ private void testANewCollectionInOneInstance() throws Exception {
SolrClient client3 = collectionClients.get(2);
SolrClient client4 = collectionClients.get(3);
- waitForRecoveriesToFinish(
- oneInstanceCollection, getCommonCloudSolrClient().getZkStateReader(), false);
- assertAllActive(oneInstanceCollection, getCommonCloudSolrClient().getZkStateReader());
+ getCommonCloudSolrClient();
+ waitForRecoveriesToFinish(oneInstanceCollection, ZkStateReader.from(cloudClient), false);
+ getCommonCloudSolrClient();
+ assertAllActive(oneInstanceCollection, ZkStateReader.from(cloudClient));
Review comment:
@haythemkh I don't know what you were doing here but I'll fix this in a minute and push a commit. Thanks @janhoy for reviewing so thoroughly!
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825251976
##########
File path: solr/core/src/test/org/apache/solr/search/join/CrossCollectionJoinQueryTest.java
##########
@@ -197,7 +197,7 @@ public void testCcJoinRoutedCollection() throws Exception {
String.format(
Locale.ROOT,
"{!join method=crossCollection zkHost=\"%s\" fromIndex=products from=product_id_s to=product_id_s}size_s:M",
- cluster.getSolrClient().getZkHost()),
+ client.getClusterStateProvider().getQuorumHosts()),
Review comment:
why? getQuorumHosts was deliberately added to be more generic
--
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
[GitHub] [solr] janhoy commented on pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #708:
URL: https://github.com/apache/solr/pull/708#issuecomment-1065399440
And the tests are GREEN!
I'll bring it up to date with latest main and then some last round of peer review...
--
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
[GitHub] [solr] janhoy commented on a change in pull request #708: SOLR-16061: CloudSolrClient changes, hide ZooKeeper
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r825473708
##########
File path: solr/CHANGES.txt
##########
@@ -639,6 +639,8 @@ and each individual module's jar will be included in its directory's lib/ folder
* SOLR-15223: Deprecate HttpSolrClient and friends, please use the Http2SolrClient equivalents (janhoy, David Smiley)
+* SOLR-16061: CloudSolrClient refactoring: Removed ZK specific methods (Haythem Khiri, David Smiley, janjoy)
Review comment:
```suggestion
* SOLR-16061: CloudSolrClient refactoring: Removed ZK specific methods (Haythem Khiri, David Smiley, janhoy)
```
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #708: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r814905630
##########
File path: solr/core/src/java/org/apache/solr/core/snapshots/SolrSnapshotsTool.java
##########
@@ -199,7 +200,7 @@ public void describeSnapshot(String collectionName, String snapshotName) {
+ " is not found for collection " + collectionName);
}
- DocCollection collectionState = solrClient.getZkStateReader().getClusterState().getCollection(collectionName);
+ DocCollection collectionState = ZkStateReader.from(solrClient).getClusterState().getCollection(collectionName);
Review comment:
In most/all cases `ZkStateReader.from(solrClient).getClusterState()` isn't needed -- just call solrClient.getClusterStateProvider().getClusterState() (or something like that). I won't repeat this comment; you can find them.
--
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