[ 
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.

Reply via email to