SOLR-16116: Use apache curator to manage the Solr Zookeeper interactions #760

HoustonPutman · 2022-03-24T17:09:55Z

https://issues.apache.org/jira/browse/SOLR-16116

This is still a WIP there are two outstanding issues:

The hadoop-auth framework requires a different version of curator
The leader election code is slightly broken, because the OnReconnect logic in ZKController expects to be called at different times, with different guarantees of watchers and ephemeral nodes than Zookeeper gives.

Might look into using curator recipes for persistent watchers and leader election, but this is a fairly complete migration to start with.

…xn is killed without an expiry

solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerRoleCmd.java

HoustonPutman · 2022-03-24T21:01:49Z

leader election code is not broken, it was that the sessionTimeout was not being populated in SolrZkClient.

risdenk · 2022-04-12T13:50:52Z

@HoustonPutman I haven't looked super closely at this, but I think that Hadoop shades curator when needed. Its possible that the Solr Hadoop classes just need to have the imports updated to use the Hadoop specific shaded curator paths. This is new with the Hadoop auth and HDFS modules in Solr when we upgraded Hadoop. I tried to switch to the Hadoop self contained shaded dependencies to avoid issues like this.

madrob · 2022-04-12T14:11:45Z

@risdenk I believe the issue currently is that for hadoop-auth we are not able to rely solely on hadoop-client-* dependencies, which are the ones that would use a relocated curator. All of the auth filter and kerberos logic and whatever else we borrow from hadoop is annotated with audience LimitedPrivate I think, so they didn't bother making a consumable client module for people to use.

HoustonPutman · 2022-04-12T14:11:54Z

Thanks for the comment @risdenk . I was talking to @madrob about it, but he hinted that there were other problems with using the shaded dependencies. However that might no longer be applicable, so I will definitely give it a shot in a week or month (things getting a bit busy haha).

dsmiley · 2023-02-19T00:05:21Z

I propose we make the loss of the reconnect boolean its own PR, plus the methods that shielded use of ZkCmdExecutor. That will make this effort here more independently reviewable and would be nice in its own right.

dsmiley · 2023-02-25T22:49:40Z

solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java

+    var clientBuilder = CuratorFrameworkFactory.builder()
+        .ensembleProvider(new FixedEnsembleProvider(zkHost))
+        .namespace(chroot)
+        .sessionTimeoutMs(zkClientTimeout)
+        .connectionTimeoutMs(clientConnectTimeout)
+        .aclProvider(this.aclProvider)
+        .authorization(zkCredentialsProvider.getCredentials())
+        .retryPolicy(retryPolicy);

+    client = clientBuilder.build();


I recently worked on an experimental hack for some days on 8x to have the Overseer use Curator for elections. One thing that I got stuck on was that Solr's tests were finding some threads lingering. Eventually I discovered that Curator has an internal executor and a way to set it on the builder runSafeService. Once I did that and tended to ensuring this executor got shot down, I didn't have that problem anymore. Just sharing this with you now in hopes it helps.

FWIW #1743 used this runSafeService stuff for the hadoop-auth changes. I think it might only come into play when there is an external zkClient provided but good to know either way.

risdenk · 2023-10-03T19:55:40Z

hadoop 3.3.6 upgraded curator to 5.2.0 - #1743. I'm working on getting that in. chatted w/ @HoustonPutman about it as well

risdenk

Overall looks great. I know it needs to be brought up to main again but 🥳

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java

risdenk · 2023-10-04T23:31:48Z

#1743 is in main and so this PR should be ready to update :D

risdenk · 2023-10-10T13:47:24Z

I merged main (after hadoop 3.3.6/curator 5.x) and then fixed up different things after that to get all the tests passing including nightly. Would appreciate another set of eyes @HoustonPutman @dsmiley

I'll look through this again to see if things can be cleaned up more, but hopefully its progress.

solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/BeforeReconnect.java

dsmiley

Note a number of feedback comments aren't addressed.

dsmiley · 2024-10-24T14:08:15Z

solr/CHANGES.txt

@@ -23,6 +23,8 @@ Improvements

 * SOLR-17077: When a shard rejoins leader election, leave previous election only once to save unneeded calls to Zookeeper. (Pierre Salagnac)

+* SOLR-16116: Apache Curator is now used to manage all Solr Zookeeper interactions. (Houston Putman, Kevin Risden, Mike Drob, David Smiley)


I think my role was minor.

BTW this PR adds a dependency to Curator from solrj-zookeeper. We should highlight solrj dependencies with more publicity than Solr server.

gradle/testing/randomization/policies/solr-tests.policy

HoustonPutman

Note a number of feedback comments aren't addressed.

Think I got all of them. Let me know if I missed any

gradle/testing/randomization/policies/solr-tests.policy

HoustonPutman · 2024-10-25T21:00:53Z

solr/solrj-zookeeper/build.gradle

@@ -32,6 +32,13 @@ dependencies {

    implementation project(':solr:solrj')

+    api('org.apache.curator:curator-client', {


Actually implementation should be ok for the curator-client. For curator-framework, that wouldn't be great because of the SolrZkClient.multi() function parameters.

HoustonPutman · 2024-10-25T21:05:21Z

solr/test-framework/build.gradle

@@ -43,6 +43,17 @@ dependencies {
  var zkExcludes = {
    exclude group: "org.apache.yetus", module: "audience-annotations"
  }
+  api('org.apache.curator:curator-client', {


I've changed all curator deps here to "implementation"

solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java

solr/core/src/test/org/apache/solr/cloud/ZkFailoverTest.java

dsmiley · 2024-10-27T03:06:11Z

solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/BadClusterTest.java

+        new CloudHttp2SolrClient.Builder(Collections.singletonList(zkHost), Optional.empty())
+            .withZkConnectTimeout(1, TimeUnit.SECONDS)
+            .build()) {
+      cloudSolrClient.getById(collection, "1");


the point was to use stream API I suppose

Ahh I guess so, but it is a slow test because of you can't override the ZkConnectTimeout of the Streaming client.

I'll revert. The test will just be slow until https://issues.apache.org/jira/browse/CURATOR-720 is implemented.

dsmiley · 2024-10-27T03:06:46Z

solr/solrj-zookeeper/build.gradle

+    implementation('org.apache.curator:curator-client', {
+        exclude group: 'org.apache.zookeeper', module: 'zookeeper'
+    })
+    api('org.apache.curator:curator-framework', {


implementation?

As I mentioned before, it's in the API contract. Solr-core actually uses it.

This is mostly a matter of taste; nothing is breaking based on this decision (I think). Solr-core already has an explicit dependency so that point is moot. I believe the impact would be some SolrJ user who adds solrj-zookeeper because they want to use ZK coordinates for CloudSolrClient. Their runtime class path will have Curator either way. But do we want such a user to have their compile-time class-path with Curator as well? Like; are they likely to use Curator explicitly? If we don't think it's likely, I think "implementation" best communicates an internal scope.

Compilation vs runtime dependency doesn't have a huge effect on users, but I'll change it.

Ok, tried it, and it won't work. Our ZkACLProvider is a sub-class of Curator's ACLProvider. Therefore, people using solrj-zookeeper and ACLs will be unable to compile without the api dependency. Changing back.

solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/DigestZkACLProvider.java

solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/DigestZkCredentialsProvider.java

solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/SolrZkClient.java

risdenk

Nice work @HoustonPutman. I found an old PR for removing connection loss stuff that conflicts but just a reminder we might want to do this cleanup after this gets merged as well

#2004

HoustonPutman · 2024-10-30T20:23:43Z

For some reason this PR breaks TestPullReplicaErrorHandling.testCloseHooksDeletedOnReconnect() on a seed that makes PRS enabled by default. From what I can tell there is an error in CreateCollectionCmd, such that PRS being turned on via the default doesn't actually work all the way. I'll try to fix that so that merging this will pass.

HoustonPutman · 2024-10-30T21:03:59Z

Well apparently actually using PRS breaks a good amount of things.... lovely. I'm going to undo that, and just add a @NoPRS tag to the test.

This reverts commit 72d9623.

iamsanjay · 2024-11-01T05:08:35Z

Possibility of relating to this one.
Build scan data https://ge.apache.org/s/63ya5yavjnh5o

Error from server at http://127.0.0.1:43991/solr: Cannot talk to ZooKeeper - Updates are disabled.

org.apache.solr.cloud.BasicDistributedZk2Test.test (:solr:core)
    Test history: https://ge.apache.org/scans/tests?search.rootProjectNames=solr-root&tests.container=org.apache.solr.cloud.BasicDistributedZk2Test&tests.test=test http://fucit.org/solr-jenkins-reports/history-trend-of-recent-failures.html#series/org.apache.solr.cloud.BasicDistributedZk2Test.test
    Test output: /tmp/src/solr/solr/core/build/test-results/test/outputs/OUTPUT-org.apache.solr.cloud.BasicDistributedZk2Test.txt
    Reproduce with: ./gradlew :solr:core:test --tests "org.apache.solr.cloud.BasicDistributedZk2Test.test" -Ptests.jvms=96 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=5426200A04BA10C -Ptests.timeoutSuite=600000! -Ptests.file.encoding=UTF-8

HoustonPutman · 2024-11-01T21:45:21Z

@iamsanjay , this was a small race condition waiting for the connection to come back. I added a wait, since I don't think this is what the test was intending to test. Commit: 0f8decd

iamsanjay · 2024-11-02T03:01:58Z

Thanks @HoustonPutman,
Do you think this one also related?
TestPullReplicaErrorHandling#testCloseHooksDeletedOnReconnect

HoustonPutman · 2024-11-04T15:45:34Z

Yes, that is absolutely related, but I thoroughly tested that before pushing.... 🙄 I'll take a look.

HoustonPutman added 7 commits March 18, 2022 16:04

Use Curator - Step 1

b4fbc8c

It compiles

e31dea0

SolrJ Tests all pass

df93241

Fix a lot of errors

3923bf6

Everything works. Just some leader election issues when the client cn…

fbcce63

…xn is killed without an expiry

Small changes.

d3949b1

Add-back-in path start index for leader election

bb7d04a

sonatype-lift bot reviewed Mar 24, 2022

View reviewed changes

solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerRoleCmd.java Outdated Show resolved Hide resolved

Change test to do stuff correctly. Add in sessionTimeout

5137cb8

Fix unrelated issue found in PR

fc773af

dsmiley reviewed Feb 25, 2023

View reviewed changes

risdenk reviewed Oct 3, 2023

View reviewed changes

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java Outdated Show resolved Hide resolved

risdenk added 2 commits October 7, 2023 20:53

Merge branch 'main' into curator

9cea7af

Fixes after merging main

539ca79

risdenk reviewed Oct 10, 2023

View reviewed changes

solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/BeforeReconnect.java Show resolved Hide resolved

risdenk requested review from dsmiley, epugh, noblepaul, stillalex, janhoy, risdenk and magibney October 10, 2023 19:12

HoustonPutman added 5 commits October 23, 2024 15:01

Some gradle dependency stuff

0ef5424

Add a changelog entry

6785d1d

Curator zk operations will wait until the connection is ready

927e261

Try treating suspended and lost the same

ea1a82a

Spotless

99e17e3

dsmiley reviewed Oct 24, 2024

View reviewed changes

HoustonPutman added 2 commits October 24, 2024 13:11

Fix overseer test

5a76cd0

Make thread a daemon thread, and fix test until curator fix

31ebd14

HoustonPutman force-pushed the curator branch from 44199a9 to 31ebd14 Compare October 25, 2024 20:55

Review comments

a59779d

HoustonPutman commented Oct 25, 2024

View reviewed changes

dsmiley reviewed Oct 27, 2024

View reviewed changes

HoustonPutman added 3 commits October 28, 2024 12:39

Review comments

f92b4d1

Some last review comments

6a1a41f

Make curator an api dependency again

b084640

dsmiley approved these changes Oct 29, 2024

View reviewed changes

risdenk approved these changes Oct 30, 2024

View reviewed changes

Fix PRS defaulting

72d9623

github-actions bot added the cat:api label Oct 30, 2024

HoustonPutman added 2 commits October 30, 2024 16:05

Revert "Fix PRS defaulting"

c71105d

This reverts commit 72d9623.

Add NoPRS to two tests

7a12fe5

github-actions bot removed the cat:api label Oct 30, 2024

HoustonPutman merged commit e5d15cc into apache:main Oct 30, 2024
4 checks passed

HoustonPutman deleted the curator branch October 30, 2024 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16116: Use apache curator to manage the Solr Zookeeper interactions #760

SOLR-16116: Use apache curator to manage the Solr Zookeeper interactions #760

HoustonPutman commented Mar 24, 2022 •

edited by risdenk

Loading

HoustonPutman commented Mar 24, 2022

risdenk commented Apr 12, 2022

madrob commented Apr 12, 2022

HoustonPutman commented Apr 12, 2022

dsmiley commented Feb 19, 2023

dsmiley Feb 25, 2023

risdenk Oct 4, 2023

risdenk commented Oct 3, 2023

risdenk left a comment

risdenk commented Oct 4, 2023

risdenk commented Oct 10, 2023

dsmiley left a comment

dsmiley Oct 24, 2024

HoustonPutman left a comment

HoustonPutman Oct 25, 2024

HoustonPutman Oct 25, 2024

dsmiley Oct 27, 2024

HoustonPutman Oct 28, 2024

HoustonPutman Oct 28, 2024

dsmiley Oct 27, 2024

HoustonPutman Oct 28, 2024

dsmiley Oct 28, 2024

HoustonPutman Oct 29, 2024

HoustonPutman Oct 29, 2024

risdenk left a comment

HoustonPutman commented Oct 30, 2024

HoustonPutman commented Oct 30, 2024

iamsanjay commented Nov 1, 2024

HoustonPutman commented Nov 1, 2024

iamsanjay commented Nov 2, 2024

HoustonPutman commented Nov 4, 2024

		@@ -23,6 +23,8 @@ Improvements

		* SOLR-17077: When a shard rejoins leader election, leave previous election only once to save unneeded calls to Zookeeper. (Pierre Salagnac)

		* SOLR-16116: Apache Curator is now used to manage all Solr Zookeeper interactions. (Houston Putman, Kevin Risden, Mike Drob, David Smiley)

		@@ -32,6 +32,13 @@ dependencies {

		implementation project(':solr:solrj')

		api('org.apache.curator:curator-client', {

SOLR-16116: Use apache curator to manage the Solr Zookeeper interactions #760

SOLR-16116: Use apache curator to manage the Solr Zookeeper interactions #760

Conversation

HoustonPutman commented Mar 24, 2022 • edited by risdenk Loading

HoustonPutman commented Mar 24, 2022

risdenk commented Apr 12, 2022

madrob commented Apr 12, 2022

HoustonPutman commented Apr 12, 2022

dsmiley commented Feb 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk commented Oct 3, 2023

risdenk left a comment

Choose a reason for hiding this comment

risdenk commented Oct 4, 2023

risdenk commented Oct 10, 2023

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoustonPutman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk left a comment

Choose a reason for hiding this comment

HoustonPutman commented Oct 30, 2024

HoustonPutman commented Oct 30, 2024

iamsanjay commented Nov 1, 2024

HoustonPutman commented Nov 1, 2024

iamsanjay commented Nov 2, 2024

HoustonPutman commented Nov 4, 2024

HoustonPutman commented Mar 24, 2022 •

edited by risdenk

Loading