You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by xyq000 <gi...@git.apache.org> on 2017/12/29 09:20:07 UTC
[GitHub] zookeeper pull request #438: ZOOKEEPER-2959: ignore accepted epoch and ack f...
GitHub user xyq000 opened a pull request:
https://github.com/apache/zookeeper/pull/438
ZOOKEEPER-2959: ignore accepted epoch and ack from observers
https://issues.apache.org/jira/browse/ZOOKEEPER-2959
After a round of elections completes, followers and observers send their accepted epochs to the leader to determine a final epoch.
Since `QuorumVerifier#containsQuorum(Set set)` does not check whether the elements of argument `set` exactly represent participants, this pull request is intended to ignore reported epochs and acks from observers for logical consistency.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/xyq000/zookeeper ZOOKEEPER-2959
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/438.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #438
----
commit 647061aa7ba1182b83b44b7f2671508012a30b4c
Author: Yongqiang Xiang <xi...@...>
Date: 2017-12-29T08:20:06Z
ignore accepted epoch and ack from observers
----
---
[GitHub] zookeeper pull request #438: ZOOKEEPER-2959: ignore accepted epoch and LEADE...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/438#discussion_r160286100
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1183,8 +1183,10 @@ public long getEpochToPropose(long sid, long lastAcceptedEpoch) throws Interrupt
if (lastAcceptedEpoch >= epoch) {
epoch = lastAcceptedEpoch+1;
}
- connectingFollowers.add(sid);
QuorumVerifier verifier = self.getQuorumVerifier();
+ if(verifier.getVotingMembers().containsKey(sid)) {
--- End diff --
I'm wondering if this logic is best suited for the `QuorumVerifier`. In other words, the quorum verifier should be able to determine if a quorum is present from a set of ids while taking into account which sids represent voting members.
---
[GitHub] zookeeper issue #438: ZOOKEEPER-2959: ignore accepted epoch and LEADERINFO a...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/438
Hi @xyq000
Thanks for the contribution. I think fixing this issues makes sense, would you please add at least one unit test to reproduce the problem?
---
[GitHub] zookeeper pull request #438: ZOOKEEPER-2959: ignore accepted epoch and LEADE...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/438#discussion_r160422756
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1183,8 +1183,10 @@ public long getEpochToPropose(long sid, long lastAcceptedEpoch) throws Interrupt
if (lastAcceptedEpoch >= epoch) {
epoch = lastAcceptedEpoch+1;
}
- connectingFollowers.add(sid);
QuorumVerifier verifier = self.getQuorumVerifier();
+ if(verifier.getVotingMembers().containsKey(sid)) {
--- End diff --
+1
---
[GitHub] zookeeper pull request #438: ZOOKEEPER-2959: ignore accepted epoch and LEADE...
Posted by shralex <gi...@git.apache.org>.
Github user shralex commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/438#discussion_r179363930
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1183,8 +1183,10 @@ public long getEpochToPropose(long sid, long lastAcceptedEpoch) throws Interrupt
if (lastAcceptedEpoch >= epoch) {
epoch = lastAcceptedEpoch+1;
}
- connectingFollowers.add(sid);
QuorumVerifier verifier = self.getQuorumVerifier();
+ if(verifier.getVotingMembers().containsKey(sid)) {
--- End diff --
If I recall correctly, the reason this wasn't done are concerns around the impact on performance - containsQuorum is called every time an ACK is received for every operation proposal. So if you need 3 asks to commit an operation, we'll be doing these checks (figuring out who's a participant and who's not} for {ACK1}, for {ACK1, ACK2} and for {ACK1, ACK2, ACK3}. This compared to comparing two ints as it stands now. So this is why it wasn't done...
---
[GitHub] zookeeper pull request #438: ZOOKEEPER-2959: ignore accepted epoch and LEADE...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/438#discussion_r179558826
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1183,8 +1183,10 @@ public long getEpochToPropose(long sid, long lastAcceptedEpoch) throws Interrupt
if (lastAcceptedEpoch >= epoch) {
epoch = lastAcceptedEpoch+1;
}
- connectingFollowers.add(sid);
QuorumVerifier verifier = self.getQuorumVerifier();
+ if(verifier.getVotingMembers().containsKey(sid)) {
--- End diff --
+1 makes sense.
---