Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: tests
    • Labels:
      None
    1. ZOOKEEPER-1987-ver8.patch
      25 kB
      Rakesh R
    2. ZOOKEEPER-1987-ver7.patch
      25 kB
      Rakesh R
    3. ZOOKEEPER-1987-ver6.patch
      23 kB
      Alexander Shraer
    4. ZOOKEEPER-1987-ver5.patch
      23 kB
      Alexander Shraer
    5. ZOOKEEPER-1987-ver4.patch
      18 kB
      Alexander Shraer

      Activity

      Hide
      Raul Gutierrez Segales added a comment -

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

      Show
      Raul Gutierrez Segales added a comment - lets follow-up here with the patch review, please see my last comment in the parent task. thanks Alexander Shraer .
      Hide
      Alexander Shraer added a comment -

      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.

      Show
      Alexander Shraer added a comment - 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.
      Hide
      Raul Gutierrez Segales added a comment -

      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.

      Show
      Raul Gutierrez Segales added a comment - 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.
      Hide
      Hadoop QA added a comment -

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

      Show
      Hadoop QA added a comment - -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.
      Hide
      Hadoop QA added a comment -

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

      Show
      Hadoop QA added a comment - -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.
      Hide
      Raul Gutierrez Segales added a comment -

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

      Show
      Raul Gutierrez Segales added a comment - +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.
      Hide
      Alexander Shraer added a comment -

      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.

      Show
      Alexander Shraer added a comment - 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.
      Hide
      Alexander Shraer added a comment -

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

      Thanks,
      Alex

      Show
      Alexander Shraer added a comment - Rakesh R does the latest patch still solve the problem on windows ? if so can you please commit it ? Thanks, Alex
      Hide
      Rakesh R added a comment -

      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)
      
      Show
      Rakesh R added a comment - 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)
      Hide
      Alexander Shraer added a comment -

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

      Show
      Alexander Shraer added a comment - good catch! attached file has a fix. Basically just checking for null in PathUtils.
      Hide
      Hadoop QA added a comment -

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

      Show
      Hadoop QA added a comment - -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.
      Hide
      Rakesh R added a comment -

      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";
              }
      
      Show
      Rakesh R added a comment - 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" ; }
      Hide
      Hadoop QA added a comment -

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

      Show
      Hadoop QA added a comment - -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.
      Hide
      Alexander Shraer added a comment -

      +1 thanks Rakesh

      Show
      Alexander Shraer added a comment - +1 thanks Rakesh
      Hide
      Rakesh R added a comment -

      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

      Show
      Rakesh R added a comment - 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
      Hide
      Hudson added a comment -

      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
      Show
      Hudson added a comment - 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

        People

        • Assignee:
          Alexander Shraer
          Reporter:
          Raul Gutierrez Segales
        • Votes:
          0 Vote for this issue
          Watchers:
          6 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development