----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/240/#review95 -----------------------------------------------------------
Great job, Vishal. I have mostly minor points about your patch. Thanks for working on it! trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java <https://reviews.apache.org/r/240/#comment174> The name of this variable doesn't bother me. trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java <https://reviews.apache.org/r/240/#comment173> Indentation seems to be incorrect for this block. trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java <https://reviews.apache.org/r/240/#comment175> If this error is fatal, then I was wondering if we shouldn't abort lookForLeader() and perhaps even propagate it up so that we kill the peer. What do you think? trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java <https://reviews.apache.org/r/240/#comment178> Indentation seems to be incorrect. trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java <https://reviews.apache.org/r/240/#comment177> Shouldn't we call sw.shutdown() from finish? trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java <https://reviews.apache.org/r/240/#comment172> Isn't it better to break this one up into multiple test cases? (Same for the previous test case.) trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java <https://reviews.apache.org/r/240/#comment180> You may consider Assert.assertTrue(msg, cnt == ecnt). trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java <https://reviews.apache.org/r/240/#comment179> If this is commented out, we should probably remove these lines from the patch. - fpj On 2011-01-07 02:21:38, Vishal Kher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/240/ > ----------------------------------------------------------- > > (Updated 2011-01-07 02:21:38) > > > Review request for zookeeper and fpj. > > > Summary > ------- > > QuorumCnxManager performed blocking socket IO at a few places. As a result, > QCM on a peer could block forever which would prevent other peers from > connecting to the blocked peer. > If the peer happens to be the leader, then it will block new peers from > becoming a follower. > > I have made changes as per ZOOKEEPER-932 > > > This addresses bug ZOOKEEPER-932. > https://issues.apache.org/jira/browse/ZOOKEEPER-932 > > > Diffs > ----- > > > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java > 1040328 > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 1040328 > trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java 1040328 > > Diff: https://reviews.apache.org/r/240/diff > > > Testing > ------- > > - ant test-core-java > - systest > - basic hand testing > - rebooted follower/leader several times > > > Thanks, > > Vishal > >
