ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1087

ForceSync VM arguement not working when set to "no"

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.3.3
    • Fix Version/s: 3.3.4, 3.4.0
    • Component/s: scripts
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Respect the "zookeeper.forceSync" system property.

      Description

      Cannot use forceSync=no to asynchronously write transaction logs. This is a critical bug, please address it ASAP. More details:

      The class org.apache.zookeeper.server.persistence.FileTxnLog initializes forceSync property in a static block. However, the static variable is defined after the static block with a default value of true. Therefore, the value of the variable can never be false. Please move the declaration of the variable before the static block.

      1. ZOOKEEPER-1087.patch
        3 kB
        Nate Putnam
      2. ZOOKEEPER-1087.patch
        2 kB
        Nate Putnam
      3. ZOOKEEPER-1087.patch
        2 kB
        Nate Putnam

        Activity

        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1218 (See https://builds.apache.org/job/ZooKeeper-trunk/1218/)
        ZOOKEEPER-1087. ForceSync VM arguement not working when set to "no"

        breed : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1137864
        Files :

        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java
        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperServerTest.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1218 (See https://builds.apache.org/job/ZooKeeper-trunk/1218/ ) ZOOKEEPER-1087 . ForceSync VM arguement not working when set to "no" breed : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1137864 Files : /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperServerTest.java
        Hide
        Benjamin Reed added a comment -

        thanx nate!
        Committed revision 1137863. (3.3 branch)
        Committed revision 1137864. (trunk)

        Show
        Benjamin Reed added a comment - thanx nate! Committed revision 1137863. (3.3 branch) Committed revision 1137864. (trunk)
        Hide
        Mahadev konar added a comment -

        ben, can you commit this? should this go into 3.4 as well?

        Show
        Mahadev konar added a comment - ben, can you commit this? should this go into 3.4 as well?
        Hide
        Benjamin Reed added a comment -

        +1 looks good

        Show
        Benjamin Reed added a comment - +1 looks good
        Hide
        Benjamin Reed added a comment -

        +1 looks good

        Show
        Benjamin Reed added a comment - +1 looks good
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483167/ZOOKEEPER-1087.patch
        against trunk revision 1136740.

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

        +1 tests included. The patch appears to include 3 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 1.3.9) warnings.

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

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

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/331//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/331//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/331//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/12483167/ZOOKEEPER-1087.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/331//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/331//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/331//console This message is automatically generated.
        Hide
        Nate Putnam added a comment -

        Fix import for Assert.assertTrue/False

        Show
        Nate Putnam added a comment - Fix import for Assert.assertTrue/False
        Hide
        Patrick Hunt added a comment -

        you want something like:

        import org.junit.Assert;
        ....
        Assert.assertTrue(tmpDir.mkdirs());

        Show
        Patrick Hunt added a comment - you want something like: import org.junit.Assert; .... Assert.assertTrue(tmpDir.mkdirs());
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12482991/ZOOKEEPER-1087.patch
        against trunk revision 1136740.

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

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

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

        -1 javac. The patch appears to cause tar ant target to fail.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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/329//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/329//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/329//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/12482991/ZOOKEEPER-1087.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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/329//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/329//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/329//console This message is automatically generated.
        Hide
        Nate Putnam added a comment -

        I'm not sure what is going on with the import of the assertion methods. The other tests in that class use assertEquals without any issue but Jenkins was complaining about assertTrue,assertFalse not being found. I don't see an explicit import for assertEquals so I'm not sure what's going on. Runs fine for me in IntelliJ and command line. Attaching the latest patch.

        Show
        Nate Putnam added a comment - I'm not sure what is going on with the import of the assertion methods. The other tests in that class use assertEquals without any issue but Jenkins was complaining about assertTrue,assertFalse not being found. I don't see an explicit import for assertEquals so I'm not sure what's going on. Runs fine for me in IntelliJ and command line. Attaching the latest patch.
        Hide
        Patrick Hunt added a comment -

        looks good, a couple suggestions:

        1) we should be able to make this variable final, no?
        2) in testForceSyncDefaultDisabled put a try/finally that resets the forceSync back to it's original value
        (you have no guarantee on the order of junit runs tests, if it runs this first your other test will fail)
        3) we don't use import * (+import static org.junit.Assert.* eclipse can do this for you automatically (organize imports)

        Show
        Patrick Hunt added a comment - looks good, a couple suggestions: 1) we should be able to make this variable final, no? 2) in testForceSyncDefaultDisabled put a try/finally that resets the forceSync back to it's original value (you have no guarantee on the order of junit runs tests, if it runs this first your other test will fail) 3) we don't use import * (+import static org.junit.Assert.* eclipse can do this for you automatically (organize imports)
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12482981/ZOOKEEPER-1087.patch
        against trunk revision 1136740.

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

        +1 tests included. The patch appears to include 3 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 1.3.9) warnings.

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

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

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/328//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/328//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/328//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/12482981/ZOOKEEPER-1087.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/328//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/328//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/328//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        fyi no need to delete the old patch(es), just upload a new version with the same name and jira handles it properly (incl seeing history)

        Show
        Patrick Hunt added a comment - fyi no need to delete the old patch(es), just upload a new version with the same name and jira handles it properly (incl seeing history)
        Hide
        Nate Putnam added a comment -

        Add import to try and fix Jenkins.

        Show
        Nate Putnam added a comment - Add import to try and fix Jenkins.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12482979/ZOOKEEPER-1087.patch
        against trunk revision 1136740.

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

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

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

        -1 javac. The patch appears to cause tar ant target to fail.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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/327//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/327//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/327//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/12482979/ZOOKEEPER-1087.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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/327//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/327//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/327//console This message is automatically generated.
        Hide
        Nate Putnam added a comment -

        Totally. New patch attached.

        Show
        Nate Putnam added a comment - Totally. New patch attached.
        Hide
        Patrick Hunt added a comment -

        it's weird that this failed (not the author's fault afaict), probably some burp on jenkins, we can try it again.

        however, before we do that, can you see a way to add a test for this? In particular we don't want to regress in future.

        Show
        Patrick Hunt added a comment - it's weird that this failed (not the author's fault afaict), probably some burp on jenkins, we can try it again. however, before we do that, can you see a way to add a test for this? In particular we don't want to regress in future.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12482948/ZOOKEEPER-1087.patch
        against trunk revision 1136740.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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 1.3.9) 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/326//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/326//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/326//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/12482948/ZOOKEEPER-1087.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 1.3.9) 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/326//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/326//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/326//console This message is automatically generated.
        Hide
        Nate Putnam added a comment -

        Moved initialization from static block to global scope.

        Show
        Nate Putnam added a comment - Moved initialization from static block to global scope.

          People

          • Assignee:
            Nate Putnam
            Reporter:
            Ankit Patel
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 5m
              5m
              Remaining:
              Remaining Estimate - 5m
              5m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development