-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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.
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
ZOOKEEPER-4886: Fix observer with small myid can't join SASL quorum #2211
base: master
Are you sure you want to change the base?
Conversation
Thanks @zichen-gan for taking care of this. Could you please create a jira ticket first? |
There was a problem hiding this 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?
sure, I create a jira ticket: https://issues.apache.org/jira/browse/ZOOKEEPER-4886 @anmolnar yes, small myid observer can't join sasl quorum. |
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. |
0e68d1c
to
dc9fe0f
Compare
There was a problem hiding this 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
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
Outdated
Show resolved
Hide resolved
...per-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthObserverTest.java
Outdated
Show resolved
Hide resolved
...per-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthObserverTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
dc9fe0f
to
84a7ec9
Compare
…rum/auth/QuorumAuthObserverTest.java
There was a problem hiding this comment.
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.
...per-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthObserverTest.java
Outdated
Show resolved
Hide resolved
...per-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthObserverTest.java
Outdated
Show resolved
Hide resolved
…rum/auth/QuorumAuthObserverTest.java
…rum/auth/QuorumAuthObserverTest.java
...per-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthObserverTest.java
Outdated
Show resolved
Hide resolved
…rum/auth/QuorumAuthObserverTest.java
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.