Mahadev konar commented on ZOOKEEPER-368:

thanks for the patch.
the patch looks good.

some comments:

- looking at the patch it seems like it would work with servers prior to 
including this patch. Did you try some testing with current servers (killing 
one at a time and brining them up  in a round robin fashion) just to make sure 
it works all fine with the current servers (not including the patch)?
- what happens if a server configured as follower is suddenly brought down and 
is made an observer and the other way around as well? Just checking to see if 
we have these scenarios covered because such mistakes are easy to make when 
setting up servers
- also it would be good to have more javadocs in the code. Its good to have 
javadocs just be explaining whats going on in each method (though we lack that 
kind of documentation in the code but I do hope we can get more javadoc)
- I think its fine to do FLE in another jira as long as it gets done. It would 
not be a useful feature if it does not run with FLE. I would have gone with 
making it work with FLE first and then trying to see if it works with LE or not.
- removing the quorums.getsize()/2 with containsQuorum() can surely be done in 
another jira.
- also performance benchmarking the code with this patch and without this patch 
so that we make sure that this patch doesnt degrade the performance in any way 
will be good to have

> 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: obs-refactor.patch, observer-refactor.patch, observers 
> sync benchmark.png, observers.patch, ZOOKEEPER-368.patch, 
> ZOOKEEPER-368.patch, ZOOKEEPER-368.patch, ZOOKEEPER-368.patch, 
> 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