----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24208/#review50414 -----------------------------------------------------------
overall looks great. a few more comments - about the new tests. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/24208/#comment88195> remove "might" src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88186> Since we know the file name here you can just read it without listing src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88187> allServers -> oldServers src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88188> this comment should be before the previous loop src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88189> testServerHasConfig returns a config str. You can make a function getVersionFromConfigStr doing something like this: Properties props = new Properties(); props.load(new StringReader(s)); return props.getProperty("version", "") and then Assert that getVersionFromConfigStr != null and version extracted from filename != null and that they are equal (for any i), and remove the "if i==0 ..." src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88190> I think you should remove this - see earlier comment src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88192> like before, I suggest to change this to compare the version returned by testServerHasConfig to the version extracted from the same server's filename link src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88198> if you take file content, append version=200000, sort, it should be the same as what "testServerHasconfig" returns when sorted. src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88199> you know the file name, no need to use "getFilesWithPrefix" src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java <https://reviews.apache.org/r/24208/#comment88200> same here src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java <https://reviews.apache.org/r/24208/#comment88184> Consider saving the result in a local list to avoid listing files again on line 101. - Alexander Shraer On Aug. 12, 2014, 6:54 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24208/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2014, 6:54 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > ------- > > ZOOKEEPER-1994 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 76f0afc > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > c4397a1 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > 0a8a45a > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 89416ca > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java > c05aa1b > src/java/test/org/apache/zookeeper/server/quorum/ReconfigRecoveryTest.java > 1a090dc > src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java e9471be > > Diff: https://reviews.apache.org/r/24208/diff/ > > > Testing > ------- > > > Thanks, > > Hongchao Deng > >
