> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, > > line 357 > > <https://reviews.apache.org/r/15568/diff/4/?file=510840#file510840line357> > > > > wouldn't this mean that we could loop forever if we got a bad state? > > sounds like an unknown rstate is unrecoverable, no?
Nope, this means that the next item in the queue will be fetch. See line aprox. 231 "response = manager.pollRecvQueue(3000, TimeUnit.MILLISECONDS);". This is the same behaviour as for a too short item in the queue, see line around 236. This means that if a new server state is added in the future, messages to old servers with this new server state will be ignored, instead of ... I don't know what could happen, but probably something gets broken. > On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, > > line 569 > > <https://reviews.apache.org/r/15568/diff/4/?file=510840#file510840line569> > > > > if this is only used for tests, shouldn't we extend this class from > > tests and add this there? Maybe it would be a bit cleaner, but it is not the goal of ZOOKEEPER-1810 to beautify all the java classes that it touches, sorry. > On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote: > > ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java, > > line 141 > > <https://reviews.apache.org/r/15568/diff/4/?file=510844#file510844line141> > > > > 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). This is how it was done in branch 3.4 and it is already committed there, so I would suggest to keep it like it is, unless it breaks something and then we fix it in both branches. > On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote: > > ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java, line > > 69 > > <https://reviews.apache.org/r/15568/diff/4/?file=510846#file510846line69> > > > > what i said above about moving this assertion inline with the test > > cases. This is how it was done in branch 3.4 and it is already committed there, so I would suggest to keep it like it is, unless it breaks something and then we fix it in both branches. - German ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15568/#review36241 ----------------------------------------------------------- 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 > >
