> On Aug. 7, 2014, 7:58 a.m., Alexander Shraer wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, > > line 478 > > <https://reviews.apache.org/r/24208/diff/1-4/?file=649202#file649202line478> > > > > 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. > > Hongchao Deng wrote: > Hi Alex. The version is loaded to dynamic properties which is parsed into > this function. The user doesn't specify a version in dynamic file but it > would still have it. I think it must have, right? > > Regarding the MainThread constructor, I have done something to handle the > case where version is added to quorumCfg: I parsed the quorumCfg to verify > that if there is any version in it and separated it out. > > Alexander Shraer wrote: > I see. We should still check that the user doesn't specify a version and > crash otherwise. Neither the static file nor the dynamic file should contain > a version (I think this means two checks). > > The thing with the test is that people will look at it as an example of > how to use the functionality. So I don't think we should be specifying a > version as part of the config if we don't want the users to do the same. > > Hongchao Deng wrote: > Good suggestion. > We should change the test to reflect appending the version to the file. > Right? > > I was thinking to > 1. have a version parameter or > 2. give a dynamic file name explicitly otherwise no dynamic file will be > created. > >
1 seems better since we can have the code that constructs a filename in one place instead of constructing it manually in every test btw notice the findbugs messages - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24208/#review49876 ----------------------------------------------------------- On Aug. 7, 2014, 5:02 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24208/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2014, 5:02 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 > >
