> On 2011-01-09 06:48:15, fpj wrote: > > Great job, Vishal. I have mostly minor points about your patch. Thanks for > > working on it!
Thanks, Flavio. I will incorporate the comments and send out a diff for review soon. Few comments below.. > On 2011-01-09 06:48:15, fpj wrote: > > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 336 > > <https://reviews.apache.org/r/240/diff/1/?file=9407#file9407line336> > > > > 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? Looking at the code this error case should never happen. But it would be good to propogate back fatal errors. How do you propose to progate the error and kill the peer? > On 2011-01-09 06:48:15, fpj wrote: > > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 896 > > <https://reviews.apache.org/r/240/diff/1/?file=9407#file9407line896> > > > > Shouldn't we call sw.shutdown() from finish? shutdown sets the running flag to false and just interrupts the thread. The thread calls finish before exiting. I found it to be cleaner to call finish from one place (while exiting the thread). > On 2011-01-09 06:48:15, fpj wrote: > > trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java, line 576 > > <https://reviews.apache.org/r/240/diff/1/?file=9409#file9409line576> > > > > If this is commented out, we should probably remove these lines from > > the patch. Opps.. > On 2011-01-09 06:48:15, fpj wrote: > > trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java, line 339 > > <https://reviews.apache.org/r/240/diff/1/?file=9409#file9409line339> > > > > Isn't it better to break this one up into multiple test cases? (Same > > for the previous test case.) ok > On 2011-01-09 06:48:15, fpj wrote: > > trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java, line 459 > > <https://reviews.apache.org/r/240/diff/1/?file=9409#file9409line459> > > > > You may consider Assert.assertTrue(msg, cnt == ecnt). ok - Vishal ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/240/#review95 ----------------------------------------------------------- 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 > >
