Skip to content

ZOOKEEPER-4886: Fix observer with small myid can't join SASL quorum #2211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Apr 20, 2025
Merged

ZOOKEEPER-4886: Fix observer with small myid can't join SASL quorum #2211

merged 6 commits into from
Apr 20, 2025
+118 −8

Conversation

zichen-gan
Copy link
Contributor

ZK BUG Like QuorumAuthObserverTest#testSmallObserverJoinSASLQuorum
When SASL Quorum like:
server.11=localhost:11223:11224:participant
server.21=localhost:11226:11227:participant
server.1=localhost:11229:11230:observer

The server.1 can't join quorum.

@anmolnar
Copy link
Contributor

anmolnar commented Nov 5, 2024

Thanks @zichen-gan for taking care of this. Could you please create a jira ticket first?

anmolnar
anmolnar reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix patch makes sense to me at first glance, but what does 'small' mean in this context? One with smaller identifier?

@zichen-gan
Copy link
Contributor Author

zichen-gan commented Nov 6, 2024 •
edited
Loading

sure, I create a jira ticket: https://issues.apache.org/jira/browse/ZOOKEEPER-4886

@anmolnar yes, small myid observer can't join sasl quorum.

@zichen-gan
Copy link
Contributor Author

Because zk always takes the initiative to connect myid with a larger myid.And startConnection() only authenticate when peer is voting, so observer can't join quorum.

@zichen-gan zichen-gan changed the title fix: small observer can't join quorum, so need use getView() ZOOKEEPER-4886: small observer can't join quorum, so need use getView() Nov 14, 2024
@zichen-gan zichen-gan force-pushed the SASLSmallObserverCanotJion branch from 0e68d1c to dc9fe0f Compare November 19, 2024 07:49
@zichen-gan zichen-gan requested a review from anmolnar November 29, 2024 07:35
kezhuw
kezhuw approved these changes Dec 21, 2024
View reviewed changes
Copy link
Member

@kezhuw kezhuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some minor changes to comments

@kezhuw kezhuw changed the title ZOOKEEPER-4886: small observer can't join quorum, so need use getView() ZOOKEEPER-4886: Fix observer with small myid can't join SASL quorum Dec 21, 2024
@kezhuw kezhuw closed this Dec 21, 2024
@kezhuw kezhuw reopened this Dec 21, 2024
anmolnar
anmolnar approved these changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Please address @kezhuw 's comments.

@zichen-gan zichen-gan force-pushed the SASLSmallObserverCanotJion branch from dc9fe0f to 84a7ec9 Compare January 22, 2025 03:09
@zichen-gan
Copy link
Contributor Author

DONE @anmolnar @kezhuw

kezhuw
kezhuw reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@kezhuw kezhuw left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test failed in both github action and jenkins due to @Timeout(value = 30). I planed to drop it as the test has watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT) already.

@anmolnar
Copy link
Contributor

anmolnar commented Mar 8, 2025

retest

@anmolnar
Copy link
Contributor

anmolnar commented Mar 8, 2025

@zichen-gan Sorry for the delay. I cannot retrigger the tests. Could you please rebase on latest master?

@kezhuw kezhuw closed this Mar 8, 2025
@kezhuw kezhuw reopened this Mar 8, 2025
@kezhuw
Copy link
Member

kezhuw commented Mar 31, 2025

I think the test is flaky somehow.

@kezhuw kezhuw force-pushed the SASLSmallObserverCanotJion branch from 58b7d6c to 467c931 Compare April 20, 2025 09:23
@kezhuw
Copy link
Member

kezhuw commented Apr 20, 2025

Rebased this to latest master to verify flakyness.

@kezhuw
Copy link
Member

kezhuw commented Apr 20, 2025

The test failed once due to flakyness of ZooKeeperServerMaxCnxnsTest.

https://github.com/apache/zookeeper/actions/runs/14560136021/job/40842324999?pr=2211#step:6:494

@kezhuw kezhuw merged commit ac19f22 into apache:master Apr 20, 2025
16 checks passed
kezhuw pushed a commit that referenced this pull request Apr 20, 2025
@zichen-gan @kezhuw
ZOOKEEPER-4886: Fix observer with small myid can't join SASL quorum
7482b95 
Reviewers: kezhuw, anmolnar
Author: zichen-gan
Closes #2211 from zichen-gan/SASLSmallObserverCanotJion

(cherry picked from commit ac19f22)
Signed-off-by: Kezhu Wang <[email protected]>
kezhuw pushed a commit that referenced this pull request Apr 20, 2025
@zichen-gan @kezhuw
ZOOKEEPER-4886: Fix observer with small myid can't join SASL quorum
6967034 
Reviewers: kezhuw, anmolnar
Author: zichen-gan
Closes #2211 from zichen-gan/SASLSmallObserverCanotJion

(cherry picked from commit ac19f22)
Signed-off-by: Kezhu Wang <[email protected]>
@kezhuw
Copy link
Member

kezhuw commented Apr 20, 2025

Merged and backported to 3.9, 3.8.

@zichen-gan Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kezhuw kezhuw kezhuw approved these changes

@anmolnar anmolnar anmolnar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zichen-gan @anmolnar @kezhuw