----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24208/#review49876 -----------------------------------------------------------
Thanks Hongchao. Some more comments below. I'll do another pass later this week. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/24208/#comment87279> can you return null in this case and then do if getVersionFromDynamicFilename() != null: setproperty("version"... the default version 0 is defined in the QuorumVerifier classes, so it seems not so good to repeat it here. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/24208/#comment87280> please remove/update the "- we change the... if reconfig happens" src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/24208/#comment87282> I think it would back up on each bootup only until a dynamic file is created the first time and linked from the static file. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/24208/#comment87283> remove !key.equals("version") from here - the user shouldn't specify a version in the dynamic file. I expect this to cause some tests to fail, such as ReconfigRecoveryTest.java Instead of having a version in the config spec tests could pass the version separately as a parameter, for example to MainThread constructor. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/24208/#comment87277> this message needs to change to reflect the new if-else condition src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java <https://reviews.apache.org/r/24208/#comment87284> you could do the same by: File f = new File(filename); return f.exists() && !f.isDirectory(); src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java <https://reviews.apache.org/r/24208/#comment87285> == 1 instead of > 0 ? - Alexander Shraer On Aug. 6, 2014, 10:09 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24208/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2014, 10:09 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 > > Diff: https://reviews.apache.org/r/24208/diff/ > > > Testing > ------- > > > Thanks, > > Hongchao Deng > >
