> 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? > > German Blanco wrote: > 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.
My motivation wasn't to beautify the class. If it is code that is only used from tests, then you are bloating the file with it has no added benefits. Not a huge deal, except it is just more code for people to go over when they next work on that file... > 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? > > German Blanco wrote: > 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. Gotcha. > 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). > > German Blanco wrote: > 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. Sure fair enough. - Raul ----------------------------------------------------------- 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 > >
