-----------------------------------------------------------
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
> 
>

Reply via email to