----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15568/#review36241 -----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/15568/#comment67187> wouldn't this mean that we could loop forever if we got a bad state? sounds like an unknown rstate is unrecoverable, no? ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/15568/#comment67188> if this is only used for tests, shouldn't we extend this class from tests and add this there? ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java <https://reviews.apache.org/r/15568/#comment67191> nit: no need for the QuorumCnxManager.Listener var, just have: ``` cnxManagers[0].listener.start(); ``` ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java <https://reviews.apache.org/r/15568/#comment67192> ditto. ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java <https://reviews.apache.org/r/15568/#comment67193> I would bring: ``` Assert.assertTrue("State is not leading.", peer.getPeerState() == ServerState.LEADING); ``` from FLETestUtils.LEThread here, it makes what's being tested more clear. Not seeing the assertions inside the test makes it a bit harder to follow what's being tested (i think). ./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java <https://reviews.apache.org/r/15568/#comment67196> nit: i would drop the comment, your code is very clean and self-explanatory. ./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java <https://reviews.apache.org/r/15568/#comment67195> nit: ditto wrt to listener var not needed. ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java <https://reviews.apache.org/r/15568/#comment67197> what i said above about moving this assertion inline with the test cases. - Raul Gutierrez Segales On March 5, 2014, 3:26 p.m., German Blanco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15568/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 3:26 p.m.) > > > Review request for zookeeper, fpj and Raul Gutierrez Segales. > > > Bugs: ZOOKEEPER-1810 > https://issues.apache.org/jira/browse/ZOOKEEPER-1810 > > > Repository: zookeeper > > > Description > ------- > > See ZOOKEEPER-1810 > > > Diffs > ----- > > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java > 1574484 > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java > 1574484 > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1574484 > ./src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1574484 > > ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java > PRE-CREATION > ./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java > PRE-CREATION > ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java > PRE-CREATION > ./src/java/test/org/apache/zookeeper/test/FLENewEpochTest.java 1574484 > ./src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 1574484 > ./src/java/test/org/apache/zookeeper/test/FLETest.java 1574484 > ./src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1574484 > ./src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1574484 > > Diff: https://reviews.apache.org/r/15568/diff/ > > > Testing > ------- > > Test included. > > > Thanks, > > German Blanco > >
