[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-368?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731366#action_12731366
 ] 

Gustavo Niemeyer commented on ZOOKEEPER-368:
--------------------------------------------

I've checked out the patch and have some minor questions/comments to make.  
Please note that I'm a "mortal" as per the terminology above, so I apologize 
for the lack of deeper understanding. :-)

[1]

+            for (FollowerHandler f : observingFollowers) {
+                if (!self.viewContains(f.sid))
+                    f.queuePacket(qp);

I'm missing the reasoning why the !self.viewContains() test is needed here.  
For the untrained eye, it sounds redundant with the fact that there are 
separate lists for observers and followers.  Might be nice to add the reasoning 
as a comment in the code, for the next pair of eyes passing over it.

[2]

+     * @param zxid
+     * @param proposal
+     */
+    public void inform(Proposal proposal) {   

There's no zxid parameter.

[3]

I'm slightly concerned about point (5) from Patrick too.  If anyone can 
subscribe as an Observer, it means that the ACLs for reading become very 
ineffective for protecting data in the ensemble (e.g. passwords, etc).  I 
understand that it's tricky to address this in the right way right now since 
there's no concept of authentication between servers, but it'd be awesome if 
such an approach could be considered in the near future for the right way of 
solving this, rather than simply whitelisting by IP.  The whitelist solution 
might be a good short term approach, but it's also a very weak approach for 
securing information.


[4]

-            forwardingFollowers.remove(follower);
-        }
+            forwardingFollowers.remove(follower);            
+        }        
(...)
-                        Socket s = ss.accept();
+                        Socket s = ss.accept();          
(...)
-                    self.setCurrentVote(result.vote);
+                    self.setCurrentVote(result.vote);                  


There are a few changes in the diff which are just adding trailing whitespace 
at the end of some lines.

[5]

The VIEWCHANGE message type isn't being sent anywhere in the patch.  Is it 
preparing for a forthcoming change?

[6]

These changes look very nice overall! (again, for an untrained eye)  Thanks for 
working on this Henry. 

> Observers
> ---------
>
>                 Key: ZOOKEEPER-368
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-368
>             Project: Zookeeper
>          Issue Type: New Feature
>          Components: quorum
>            Reporter: Flavio Paiva Junqueira
>            Assignee: Henry Robinson
>         Attachments: ZOOKEEPER-368.patch, ZOOKEEPER-368.patch, 
> ZOOKEEPER-368.patch, ZOOKEEPER-368.patch
>
>
> Currently, all servers of an ensemble participate actively in reaching 
> agreement on the order of ZooKeeper transactions. That is, all followers 
> receive proposals, acknowledge them, and receive commit messages from the 
> leader. A leader issues commit messages once it receives acknowledgments from 
> a quorum of followers. For cross-colo operation, it would be useful to have a 
> third role: observer. Using Paxos terminology, observers are similar to 
> learners. An observer does not participate actively in the agreement step of 
> the atomic broadcast protocol. Instead, it only commits proposals that have 
> been accepted by some quorum of followers.
> One simple solution to implement observers is to have the leader forwarding 
> commit messages not only to followers but also to observers, and have 
> observers applying transactions according to the order followers agreed upon. 
> In the current implementation of the protocol, however, commit messages do 
> not carry their corresponding transaction payload because all servers 
> different from the leader are followers and followers receive such a payload 
> first through a proposal message. Just forwarding commit messages as they 
> currently are to an observer consequently is not sufficient. We have a couple 
> of options:
> 1- Include the transaction payload along in commit messages to observers;
> 2- Send proposals to observers as well.
> Number 2 is simpler to implement because it doesn't require changing the 
> protocol implementation, but it increases traffic slightly. The performance 
> impact due to such an increase might be insignificant, though.
> For scalability purposes, we may consider having followers also forwarding 
> commit messages to observers. With this option, observers can connect to 
> followers, and receive messages from followers. This choice is important to 
> avoid increasing the load on the leader with the number of observers. 

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