lets follow-up here with the patch review, please see my last comment in the parent task. thanks Alexander Shraer.

Thanks for the comments, uploading an updated patch.
Rakesh R, I changed your fix slightly, does my fix solve the problem you saw on windows ? the change aims to avoid doing "replace" every time we write the dynamic config file - instead doing it once when the file name is set.

This is slightly outside of the scope of this patch (and a nit) but if you have the cycles moving all the file path conversions to one util function would clean up things a bit, see:

$ git grep "osname.toLowerCase().contains"
src/java/test/org/apache/zookeeper/server/InvalidSnapCountTest.java:            if (osname.toLowerCase().contains("windows")) 
src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java:            if (osname.toLowerCase().contains("windows"
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java:            if (osname.toLowerCase().contains("window
src/java/test/org/apache/zookeeper/server/util/DynamicConfigBCTest.java:            if (osname.toLowerCase().contains("windows

so having a normalizeFilePath() or such would make it easier to maintain.

Less lines of code is always nice

It's also such a small change that another JIRA is probably overkill.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12657642/ZOOKEEPER-1987-ver4.patch
against trunk revision 1612906.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 9 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2229//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2229//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2229//console

This message is automatically generated.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12657650/ZOOKEEPER-1987-ver5.patch
against trunk revision 1612906.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 15 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2230//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2230//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2230//console

This message is automatically generated.

+1, I think it looks great. Thanks Alexander Shraer. One last nit though (feel free to leave it): since :

PathUtils.normalizeFileSystemPath

is already a static method inside PathUtils, you might as well just call it normalize (it is implicit it refers to paths):

PathUtils.normalize

Thanks for the quick updates.

Thanks Raul. I'd rather leave as is - PathUtils refers to znode paths, that's why I named it this way to make clear we mean a file system directory.

Rakesh R does the latest patch still solve the problem on windows ?
if so can you please commit it ?

Thanks,
Alex

Alexander Shraer IMHO we need to handle one more case. I'm getting the following exception, this happens when starting the server without the dynamic config file.

2014-07-25 10:37:23,559 [myid:1] - INFO  [Thread-1:QuorumPeer@1253] - initLimit set to 10
2014-07-25 10:37:23,559 [myid:2] - ERROR [Thread-2:QuorumPeerTestBase$MainThread@162] - unexpected exception in run
java.lang.NullPointerException
	at org.apache.zookeeper.common.PathUtils.normalizeFileSystemPath(PathUtils.java:108)
	at org.apache.zookeeper.server.quorum.QuorumPeer.setDynamicConfigFilename(QuorumPeer.java:322)
	at org.apache.zookeeper.server.quorum.QuorumPeerMain.runFromConfig(QuorumPeerMain.java:157)
	at org.apache.zookeeper.server.quorum.QuorumPeerMain.initializeAndRun(QuorumPeerMain.java:120)
	at org.apache.zookeeper.server.quorum.QuorumPeerTestBase$MainThread.run(QuorumPeerTestBase.java:159)
	at java.lang.Thread.run(Thread.java:619)

good catch! attached file has a fix. Basically just checking for null in PathUtils.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12657785/ZOOKEEPER-1987-ver6.patch
against trunk revision 1613328.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 15 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2232//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2232//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2232//console

This message is automatically generated.

Alexander Shraer Since its doing the path conversion outside the QPC#writeDynamicConfig(), its missed in one more case. Please see the below snippet where its forming the dynamic config in backward compatible way. I've modified the patch little bit and attached the same. Apart from this I'm OK with the patch.

QPC#writeDynamicConfig()

        if (configBackwardCompatibilityMode) {
            dynamicConfigFilename = configFileStr + ".dynamic";
        }

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12657797/ZOOKEEPER-1987-ver7.patch
against trunk revision 1613328.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 15 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2233//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2233//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2233//console

This message is automatically generated.

+1 thanks Rakesh

Attached committed patch, here I did small changes in PathUtils.java - added javadoc for the conversion method and removed unwanted IllegalArgumentException clause.

Thanks Raul Gutierrez Segales for the reviews & Alexander Shraer for the fix.

Committed to trunk : http://svn.apache.org/viewvc?view=revision&revision=1613553

SUCCESS: Integrated in ZooKeeper-trunk #2387 (See https://builds.apache.org/job/ZooKeeper-trunk/2387/)
ZOOKEEPER-1988. new test patch to verify dynamic reconfig backward compatibility (Alexander Shraer via rakeshr) (rakeshr: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1613553)

• /zookeeper/trunk/CHANGES.txt
• /zookeeper/trunk/src/java/main/org/apache/zookeeper/common/PathUtils.java
• /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
• /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/InvalidSnapCountTest.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/DynamicConfigBackwardCompatibilityTest.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/util/DynamicConfigBCTest.java