[ https://issues.apache.org/jira/browse/ZOOKEEPER-29?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12699796#action_12699796 ]
Mahadev konar edited comment on ZOOKEEPER-29 at 4/16/09 11:33 AM: ------------------------------------------------------------------ sorry to be commenting late on the patch flavio, - minor nit - there is an unneccesary change in AuthFastLeaderElection.java - I see changes to FastLeaderElection but no changes in LeaderElection. Shouldnt both of them change. - also this breaks backward comtability with servers, should this go into 4.0? or is 3.2 fine? - isnt the sid sent only once so why do we have that in a loop - {code} break; case Leader.SID: bb = ByteBuffer.wrap(qp.getData()); this.sid = bb.getLong(); break; {code} do we expect the followers to keep sending the SID? - I think this code is wrong in Leader.java (line 297 or so) {code} HashSet<Long> followerSet = new HashSet<Long>(); for(FollowerHandler f : followers) followerSet.add(f.getSid()); if (self.getQuorumVerifier().containsQuorum(followerSet)) { //if (followers.size() >= self.quorumPeers.size() / 2) { LOG.warn("Enough followers present. "+ "Perhaps the init {code} The reason being the sid is always set to 0 even if the follower hasnt been able to be synced with the leader and sent back the sid -- which is the above case. So mostly the above statement would never execute since the sid is only sent after the follower synchronizes the database. Meaning that your code would not be able to estimate if you had the right set of followers with the SID. Why isnt the SID sent as the first packet when syncing up with the leader as a payload? Why do we need another protocol change? - also it would be good to have checking in groups and weights (mostly for mistakes that people would make). You can see that it gets really easy to make mistakes with this configuration pattern. We should have more error checking (like serverid belongs to some unique group) and also in cases like {code} if((serverWeight.size() > 0) && (serverGroup.size() > 0) && (servers.size() == serverGroup.size())){ {code} where only one or two of the above statements are true, we should fail saying that some problem on the config files rather than resorting to majority config. We can do it in seperate jira. - sorry for being naive, you mentioned in the comments that "This gives us quorums of size 4, whereas majority quorums would require 5." . What are the implications of this? Meaning is their anycase where the servers can diverge on the databases? was (Author: mahadev): sorry to be commenting late on the patch flavio, - minor nit - there is an unneccesary change in AuthFastLeaderElection.java - I see changes to FastLeaderElection but no changes in LeaderElection. Shouldnt both of them change. - also this breaks backward comtability with servers, should this go into 4.0? or is 3.2 fine? - isnt the sid sent only once so why do we have that in a loop - {code} break; case Leader.SID: bb = ByteBuffer.wrap(qp.getData()); this.sid = bb.getLong(); break; {code} do we expect the followers to keep sending the SID? - I think this code is wrong in Leader.java (line 297 or so) {code} HashSet<Long> followerSet = new HashSet<Long>(); for(FollowerHandler f : followers) followerSet.add(f.getSid()); if (self.getQuorumVerifier().containsQuorum(followerSet)) { //if (followers.size() >= self.quorumPeers.size() / 2) { LOG.warn("Enough followers present. "+ "Perhaps the init {code} The reason being the sid is always get to 0 even if the follower hasnt been able to be synced with the leader and sent back the sid -- which is the above case. So mostly the above statement would never execute since the sid is only sent after the follower synchronizes the database. Meaning that your code would not be able to estimate if you had the right set of followers with the SID. Why isnt the SID sent as the first packet when syncing up with the leader as a payload? Why do we need another protocol change? - also it would be good to have checking in groups and weights (mostly for mistakes that people would make). You can see that it gets really easy to make mistakes with this configuration pattern. We should have more error checking (like serverid belongs to some unique group) and also in cases like {code} if((serverWeight.size() > 0) && (serverGroup.size() > 0) && (servers.size() == serverGroup.size())){ {code} where only one or two of the above statements are true, we should fail saying that some problem on the config files rather than resorting to majority config. We can do it in seperate jira. - sorry for being naive, you mentioned in the comments that "This gives us quorums of size 4, whereas majority quorums would require 5." . What are the implications of this? Meaning is their anycase where the servers can diverge on the databases? > Flexible quorums > ---------------- > > Key: ZOOKEEPER-29 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-29 > Project: Zookeeper > Issue Type: New Feature > Components: server > Reporter: Patrick Hunt > Assignee: Flavio Paiva Junqueira > Fix For: 3.2.0 > > Attachments: ZOOKEEPER-29.patch, ZOOKEEPER-29.patch, > ZOOKEEPER-29.patch, ZOOKEEPER-29.patch, ZOOKEEPER-29.patch, > ZOOKEEPER-29.patch, ZOOKEEPER-29.patch > > > Moved from SourceForge to Apache. > http://sourceforge.net/tracker/index.php?func=detail&aid=1938782&group_id=209147&atid=1008547 -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.