ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1390

some expensive debug code not protected by a check for debug

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.4, 3.5.0
    • Component/s: server
    • Labels:
      None

      Description

      there is some expensive debug code in DataTree.processTxn() that formats transactions for debugging that are very expensive but are only used when errors happen and when debugging is turned on.

        Activity

        Hide
        Benjamin Reed added a comment -

        this fixes the performance issue. i found that the improvement is anywhere from 5% (with 100% reads) to almost 100% (with 100% writes and 3 servers)

        no tests since this is not a bug fix and does not add functionality.

        Show
        Benjamin Reed added a comment - this fixes the performance issue. i found that the improvement is anywhere from 5% (with 100% reads) to almost 100% (with 100% writes and 3 servers) no tests since this is not a bug fix and does not add functionality.
        Hide
        Hadoop QA added a comment -

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

        +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 passed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/955//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/955//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/955//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/12514078/ZOOKEEPER-1390.patch against trunk revision 1240959. +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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/955//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/955//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/955//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Do you think we might want to leave in those more descriptive debug strings but guarded by an if (LOG.isDebugEnabled())? I don't care either way but it might be useful.

        Otherwise this looks good to me, good catch.

        Show
        Camille Fournier added a comment - Do you think we might want to leave in those more descriptive debug strings but guarded by an if (LOG.isDebugEnabled())? I don't care either way but it might be useful. Otherwise this looks good to me, good catch.
        Hide
        Benjamin Reed added a comment -

        that's a good point camille. i thought it looked kind of ugly scattering all the if(...) around the code, but the debug messages, do look better than the rather raw looking messages you get from the toString() methods of the txnhdr and txn. i also don't have a strong opinion, so i'd like to leave as is, but if anyone feels strongly the other way, i'm fine spinning a new patch.

        Show
        Benjamin Reed added a comment - that's a good point camille. i thought it looked kind of ugly scattering all the if(...) around the code, but the debug messages, do look better than the rather raw looking messages you get from the toString() methods of the txnhdr and txn. i also don't have a strong opinion, so i'd like to leave as is, but if anyone feels strongly the other way, i'm fine spinning a new patch.
        Hide
        Camille Fournier added a comment -

        Ok, I'm pretty flexible on it. Added it also as a 3.4.X issue since it's present there as well.

        Show
        Camille Fournier added a comment - Ok, I'm pretty flexible on it. Added it also as a 3.4.X issue since it's present there as well.
        Hide
        Mahadev konar added a comment -

        Good to have this in 3.4.4. Camille want to go ahead and commit this to trunk and 3.4 branch?

        Show
        Mahadev konar added a comment - Good to have this in 3.4.4. Camille want to go ahead and commit this to trunk and 3.4 branch?
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1495 (See https://builds.apache.org/job/ZooKeeper-trunk/1495/)
        ZOOKEEPER-1390. some expensive debug code not protected by a check for debug (breed via camille) (Revision 1301947)

        Result = SUCCESS
        camille : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1301947
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1495 (See https://builds.apache.org/job/ZooKeeper-trunk/1495/ ) ZOOKEEPER-1390 . some expensive debug code not protected by a check for debug (breed via camille) (Revision 1301947) Result = SUCCESS camille : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1301947 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java
        Hide
        Camille Fournier added a comment -

        checked in to trunk and 3.4.4

        Show
        Camille Fournier added a comment - checked in to trunk and 3.4.4

          People

          • Assignee:
            Benjamin Reed
            Reporter:
            Benjamin Reed
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development