You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Hongchao Deng <hd...@cloudera.com> on 2014/08/02 20:06:18 UTC
Review Request 24208: ZOOKEEPER-1994
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50261
-----------------------------------------------------------
I also found a "problem" that the dynamic file config should be absolute path. Otherwise changing the working directory doesn't work.
- Hongchao Deng
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 9, 2014, 1:49 p.m., Alexander Shraer wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1340
> > <https://reviews.apache.org/r/24208/diff/1-10/?file=649201#file649201line1340>
> >
> > Looking at this change again it seems that this can potentially change the behavior of tests that previously started without a static config file but have a dynamic file. Before the change they would continue writing out the dynamic file when the configuration changed. After the change this whole block of code is not run , so no dynamic file changes would happen.
> >
> > One issue is that you're always basing the dynamic filename on the static filename but in this case you don't have the static one. Perhaps you should have dynamicConfigFilenamePrefix or something like that ?
> >
> > so instead of using configFilename + ".dynamic" + number maybe you should do dynamicConfigFilenamePrefix + number if dynamicConfigFilenamePrefix is not null.
> >
> > See condition of the "if" statement before the change. Perhaps you can propose a better solution.
> >
> >
>
> Alexander Shraer wrote:
> on second thought it may be ok as it is - the test that I have in mind is ReconfigTest.java, but this test is not testing recovery, so its probably fine if it doesn't write configuration stuff on disk.
So instead of making more a decisive change, I would make a getDynamicFilenamePrefix(), which still takes configFilename + ".dynamic" for this moment. How did that go for you?
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50111
-----------------------------------------------------------
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On Aug. 9, 2014, 1:49 p.m., Alexander Shraer wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1340
> > <https://reviews.apache.org/r/24208/diff/1-10/?file=649201#file649201line1340>
> >
> > Looking at this change again it seems that this can potentially change the behavior of tests that previously started without a static config file but have a dynamic file. Before the change they would continue writing out the dynamic file when the configuration changed. After the change this whole block of code is not run , so no dynamic file changes would happen.
> >
> > One issue is that you're always basing the dynamic filename on the static filename but in this case you don't have the static one. Perhaps you should have dynamicConfigFilenamePrefix or something like that ?
> >
> > so instead of using configFilename + ".dynamic" + number maybe you should do dynamicConfigFilenamePrefix + number if dynamicConfigFilenamePrefix is not null.
> >
> > See condition of the "if" statement before the change. Perhaps you can propose a better solution.
> >
> >
on second thought it may be ok as it is - the test that I have in mind is ReconfigTest.java, but this test is not testing recovery, so its probably fine if it doesn't write configuration stuff on disk.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50111
-----------------------------------------------------------
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50111
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/24208/#comment87677>
Looking at this change again it seems that this can potentially change the behavior of tests that previously started without a static config file but have a dynamic file. Before the change they would continue writing out the dynamic file when the configuration changed. After the change this whole block of code is not run , so no dynamic file changes would happen.
One issue is that you're always basing the dynamic filename on the static filename but in this case you don't have the static one. Perhaps you should have dynamicConfigFilenamePrefix or something like that ?
so instead of using configFilename + ".dynamic" + number maybe you should do dynamicConfigFilenamePrefix + number if dynamicConfigFilenamePrefix is not null.
See condition of the "if" statement before the change. Perhaps you can propose a better solution.
- Alexander Shraer
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50250
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment87910>
nits about style:
* space between if and (
* if statements always have {} in their body (i.e.: if (cond) { })
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment87913>
nit: spaces between if and ( and in the cond
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment87915>
ditto
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java
<https://reviews.apache.org/r/24208/#comment87916>
ditto
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java
<https://reviews.apache.org/r/24208/#comment87917>
ditto
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java
<https://reviews.apache.org/r/24208/#comment87919>
no need for the surround ()s
- Raul Gutierrez Segales
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50272
-----------------------------------------------------------
So I keep using configFileStr + ".dynamic" ... to make change as small as possible.
- Hongchao Deng
On Aug. 12, 2014, 12:58 a.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 12:58 a.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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 13, 2014, 10:31 p.m.)
Review request for zookeeper.
Changes
-------
Alex's version
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 13, 2014, 5:20 p.m.)
Review request for zookeeper.
Changes
-------
Fix as per Alex's comments
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 13, 2014, 6:36 a.m., Alexander Shraer wrote:
> > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java, lines 184-193
> > <https://reviews.apache.org/r/24208/diff/14/?file=658731#file658731line184>
> >
> > 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
Yes. I will make sure compare the version from testServerHasConfig (quorum verifier). I would like to also include this check:
1. checks that versions of filenames are all the same.
2. second version is larger than first.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50414
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 13, 2014, 6:36 a.m., Alexander Shraer wrote:
> > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java, line 294
> > <https://reviews.apache.org/r/24208/diff/14/?file=658731#file658731line294>
> >
> > if you take file content, append version=200000, sort, it should be the same as what "testServerHasconfig" returns when sorted.
Well, dynamic content are sorted before written to file.
But neither testServerHasConfig nor quorumverifier#toString do sorting. So I didn't make such assumptions.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50414
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 13, 2014, 6:36 a.m., Alexander Shraer wrote:
> > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java, lines 150-156
> > <https://reviews.apache.org/r/24208/diff/14/?file=658731#file658731line150>
> >
> > I think you should remove this - see earlier comment
This checks that versions of all servers are the same.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50414
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On Aug. 13, 2014, 6:36 a.m., Alexander Shraer wrote:
> > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java, lines 150-156
> > <https://reviews.apache.org/r/24208/diff/14/?file=658731#file658731line150>
> >
> > I think you should remove this - see earlier comment
>
> Hongchao Deng wrote:
> This checks that versions of all servers are the same.
I understand, but instead you could get the expected config from one of the servers first, by using getVersionFromConfigStr, and then this instead of this code here just check that the config in the file + version in the filename matches expected config.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50414
-----------------------------------------------------------
On Aug. 13, 2014, 5:20 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2014, 5:20 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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 12, 2014, 4:34 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 12, 2014, 4:04 p.m.)
Review request for zookeeper.
Changes
-------
refactor: remove 'memFilename' in QuorumPeer constructor.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 12, 2014, 12:58 a.m.)
Review request for zookeeper.
Changes
-------
Just remove 'dynamicConfigFile' variable in QuorumPeer.
Fix 'if' formatting as per Raul's comments.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 9, 2014, 3:06 p.m., Alexander Shraer wrote:
> > One consequence of the current implementation is that the user doesn't have control over the name of the dynamic file.
> > if I start zookeeper and name my static file "static.file" and my dynamic file "dynamic.file"
> > then after the server starts up the new dynamic file will be called "static.file.dynamic.10000" and not "dynamic.file.10000".
> >
> > What do you think ?
> >
> > If you leave the current implementation as is, then it seems like QuorumPeer.dynamicConfigFilename
> > is no longer used. If so shouldn't we remove it completely ?
> >
> >
> >
> >
>
> Hongchao Deng wrote:
> Thanks for comments.
>
> So either one sounds good to me.
> I will try to work out something based on dynamic file name ("dynamic.file.10000").
>
> So one question is that should dynamic.next file be static or based on dynamic file name?
> The latter one could help detect corner case like "null.next" but former one is safer.
After a second thought, it might be better to use predefined "zoo.cfg.dynamic.1000" because this eliminates possible bad path name (too long, maybe?).
I don't think users will be affected that much for the using a different name.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50113
-----------------------------------------------------------
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 9, 2014, 3:06 p.m., Alexander Shraer wrote:
> > One consequence of the current implementation is that the user doesn't have control over the name of the dynamic file.
> > if I start zookeeper and name my static file "static.file" and my dynamic file "dynamic.file"
> > then after the server starts up the new dynamic file will be called "static.file.dynamic.10000" and not "dynamic.file.10000".
> >
> > What do you think ?
> >
> > If you leave the current implementation as is, then it seems like QuorumPeer.dynamicConfigFilename
> > is no longer used. If so shouldn't we remove it completely ?
> >
> >
> >
> >
Thanks for comments.
So either one sounds good to me.
I will try to work out something based on dynamic file name ("dynamic.file.10000").
So one question is that should dynamic.next file be static or based on dynamic file name?
The latter one could help detect corner case like "null.next" but former one is safer.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50113
-----------------------------------------------------------
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review50113
-----------------------------------------------------------
One consequence of the current implementation is that the user doesn't have control over the name of the dynamic file.
if I start zookeeper and name my static file "static.file" and my dynamic file "dynamic.file"
then after the server starts up the new dynamic file will be called "static.file.dynamic.10000" and not "dynamic.file.10000".
What do you think ?
If you leave the current implementation as is, then it seems like QuorumPeer.dynamicConfigFilename
is no longer used. If so shouldn't we remove it completely ?
- Alexander Shraer
On Aug. 8, 2014, 2:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 2: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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 8, 2014, 2:54 p.m.)
Review request for zookeeper.
Changes
-------
fix flaky test
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 10:28 p.m.)
Review request for zookeeper.
Changes
-------
fix test
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 9:38 p.m.)
Review request for zookeeper.
Changes
-------
pass ReconfigRecoveryTest
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 9:20 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 9:16 p.m.)
Review request for zookeeper.
Changes
-------
fix version stuff in test
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
fixed regarding Alex comments
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> 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.
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.
- Hongchao
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> 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.
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.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review49876
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> 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.
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.
- 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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> 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.
>
>
>
> Alexander Shraer wrote:
> 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
Regarding the check, I think static file already had.
For dynamic file, after loading the file and before I parse the version from filename, I can check it. This requires little changes.
- Hongchao
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> 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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 6, 2014, 2:13 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
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/ReconfigLegacyTest.java c05aa1b
src/java/test/org/apache/zookeeper/server/quorum/ReconfigRecoveryTest.java 1a090dc
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/
-----------------------------------------------------------
(Updated Aug. 4, 2014, 10:26 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-1994
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
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
Diff: https://reviews.apache.org/r/24208/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On Aug. 2, 2014, 6:59 p.m., Alexander Shraer wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1356
> > <https://reviews.apache.org/r/24208/diff/1/?file=649201#file649201line1356>
> >
> > I think instead of needEraseClientInfoFromStaticConfig you shold just have "getAllMembers().get(getId()).clientAddr != null" here. See comments below first.
sorry probably getAllMembers().get(getId()) != null && getAllMembers().get(getId()).clientAddr != null.
Does this make sense ?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review49425
-----------------------------------------------------------
On Aug. 2, 2014, 6:06 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2014, 6:06 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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Aug. 2, 2014, 6:59 p.m., Alexander Shraer wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 59
> > <https://reviews.apache.org/r/24208/diff/1/?file=649202#file649202line59>
> >
> > does this need to be public ?
It was used in QuorumPeer.java
{code}
public String getNextDynamicConfigFilename() {
return configFilename + QuorumPeerConfig.nextDynamicConfigFileSuffix;
}
{code}
> On Aug. 2, 2014, 6:59 p.m., Alexander Shraer wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1356
> > <https://reviews.apache.org/r/24208/diff/1/?file=649201#file649201line1356>
> >
> > I think instead of needEraseClientInfoFromStaticConfig you shold just have "getAllMembers().get(getId()).clientAddr != null" here. See comments below first.
>
> Alexander Shraer wrote:
> sorry probably getAllMembers().get(getId()) != null && getAllMembers().get(getId()).clientAddr != null.
> Does this make sense ?
Made sense to me.
I think instead of removing the function we can just change the function.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review49425
-----------------------------------------------------------
On Aug. 2, 2014, 6:06 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2014, 6:06 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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 24208: ZOOKEEPER-1994
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24208/#review49425
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/24208/#comment86491>
if we no longer need the configBackwardCompatibility flag, lets remove it from everywhere.
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/24208/#comment86488>
use getDynamicConfigFilename()
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/24208/#comment86492>
I think instead of needEraseClientInfoFromStaticConfig you shold just have "getAllMembers().get(getId()).clientAddr != null" here. See comments below first.
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment86493>
does this need to be public ?
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment86494>
add a key="version", value= version number extracted from the dymicConfigFileStr into dynamicCfg - see my comment on the jira.
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment86489>
indentation
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/24208/#comment86490>
I just noticed that this would delete the dynamic stuff from the static file (server weight, etc) before the dynamic file actually exists. I missed this bug in 1992.
Here's my suggestion - remove this line here.
I think that when you invoke editStaticConfig after you invoke writeDynamicConfig in QuorumPeer it should just remove the clientPort stuff, because at that time the dynamic file already exists. The flag should be a bit different though - I'll comment there.
- Alexander Shraer
On Aug. 2, 2014, 6:06 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24208/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2014, 6:06 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
>
> Diff: https://reviews.apache.org/r/24208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>