ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1176

Remove dead code and basic cleanup in DataTree

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      cleanup, cleancode

      Description

      • DataTree members scount, initialized and method listACLEquals are never used
      • transform if(!C) B else A to if(C) A else B (removes one indirection to follow for the brain)
      • remove unused imports and one annotation
      • add method getApproximateDataSize to DataNode (I work towards an immutable DataNode without public properties)
      • move assignments (lastPrefix = getMaxPrefixWithQuota(path)) out of if statements
      • combine nested if statements: if A if B then C => if A && B => C
      • make ACL maps private and add getAclSize() to hide implementation details of the ACLs.
      1. ZOOKEEPER-1176.patch
        67 kB
        Thomas Koch
      2. ZOOKEEPER-1176.patch
        13 kB
        Thomas Koch
      3. ZOOKEEPER-1176.patch
        13 kB
        Thomas Koch
      4. ZOOKEEPER-1176.patch
        13 kB
        Thomas Koch
      5. ZOOKEEPER-1176.patch
        13 kB
        Thomas Koch

        Activity

        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1306 (See https://builds.apache.org/job/ZooKeeper-trunk/1306/)
        ZOOKEEPER-1176. Remove dead code and basic cleanup in DataTree (Thomas Koch via phunt)

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

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataNode.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZKDatabase.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FollowerResyncConcurrencyTest.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RestoreCommittedLogTest.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1306 (See https://builds.apache.org/job/ZooKeeper-trunk/1306/ ) ZOOKEEPER-1176 . Remove dead code and basic cleanup in DataTree (Thomas Koch via phunt) phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171879 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataNode.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZKDatabase.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FollowerResyncConcurrencyTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RestoreCommittedLogTest.java
        Hide
        Patrick Hunt added a comment -

        committed to trunk (3.5.0) thanks Thomas.

        Show
        Patrick Hunt added a comment - committed to trunk (3.5.0) thanks Thomas.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 6 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/545//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/545//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/545//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/12494592/ZOOKEEPER-1176.patch against trunk revision 1170886. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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/545//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/545//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/545//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/
        -----------------------------------------------------------

        (Updated 2011-09-15 09:06:50.251249)

        Review request for zookeeper.

        Summary
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-15 09:06:50.251249) Review request for zookeeper. Summary -------
        Hide
        Thomas Koch added a comment -

        missed a compiler error in the last patch.

        Additionally fixed that two methods returned implementation types instead of Interfaces. (HashSet vs Set, LinkedList vs List) - This required subsequent changes in the calling classes that expected the implementation types but used only the Interfaces.

        Show
        Thomas Koch added a comment - missed a compiler error in the last patch. Additionally fixed that two methods returned implementation types instead of Interfaces. (HashSet vs Set, LinkedList vs List) - This required subsequent changes in the calling classes that expected the implementation types but used only the Interfaces.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-14 18:34:18, Patrick Hunt wrote:

        > src/java/main/org/apache/zookeeper/server/DataTree.java, line 160

        > <https://reviews.apache.org/r/1772/diff/3/?file=40496#file40496line160>

        >

        > while you're here, add javadoc?

        I'm not ready yet with the DataTree. JavaDocs that I'd add now would get obsolete by the next patch.

        On 2011-09-14 18:34:18, Patrick Hunt wrote:

        > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java, line 265

        > <https://reviews.apache.org/r/1772/diff/3/?file=40498#file40498line265>

        >

        > why is this dropped? (I didn't see it noted in the jira)

        The setDataTreeInit method has been removed because it only did set the DataTree.initialized property which was never ever read again.

        • Thomas

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/#review1896
        -----------------------------------------------------------

        On 2011-09-15 08:31:34, Thomas Koch wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1772/

        -----------------------------------------------------------

        (Updated 2011-09-15 08:31:34)

        Review request for zookeeper.

        Summary

        -------

        .

        This addresses bug ZOOKEEPER-1176.

        https://issues.apache.org/jira/browse/ZOOKEEPER-1176

        Diffs

        -----

        src/java/main/org/apache/zookeeper/server/DataNode.java d839a74

        src/java/main/org/apache/zookeeper/server/DataTree.java 27338d1

        src/java/main/org/apache/zookeeper/server/ZKDatabase.java 6bad5fc

        src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 2d9b104

        Diff: https://reviews.apache.org/r/1772/diff

        Testing

        -------

        Thanks,

        Thomas

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-14 18:34:18, Patrick Hunt wrote: > src/java/main/org/apache/zookeeper/server/DataTree.java, line 160 > < https://reviews.apache.org/r/1772/diff/3/?file=40496#file40496line160 > > > while you're here, add javadoc? I'm not ready yet with the DataTree. JavaDocs that I'd add now would get obsolete by the next patch. On 2011-09-14 18:34:18, Patrick Hunt wrote: > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java, line 265 > < https://reviews.apache.org/r/1772/diff/3/?file=40498#file40498line265 > > > why is this dropped? (I didn't see it noted in the jira) The setDataTreeInit method has been removed because it only did set the DataTree.initialized property which was never ever read again. Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/#review1896 ----------------------------------------------------------- On 2011-09-15 08:31:34, Thomas Koch wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-15 08:31:34) Review request for zookeeper. Summary ------- . This addresses bug ZOOKEEPER-1176 . https://issues.apache.org/jira/browse/ZOOKEEPER-1176 Diffs ----- src/java/main/org/apache/zookeeper/server/DataNode.java d839a74 src/java/main/org/apache/zookeeper/server/DataTree.java 27338d1 src/java/main/org/apache/zookeeper/server/ZKDatabase.java 6bad5fc src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 2d9b104 Diff: https://reviews.apache.org/r/1772/diff Testing ------- Thanks, Thomas
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/
        -----------------------------------------------------------

        (Updated 2011-09-15 08:31:34.680636)

        Review request for zookeeper.

        Summary
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-15 08:31:34.680636) Review request for zookeeper. Summary -------
        Hide
        Thomas Koch added a comment -

        updated patch to new trunk

        Show
        Thomas Koch added a comment - updated patch to new trunk
        Hide
        Hadoop QA added a comment -

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

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/542//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/12494417/ZOOKEEPER-1176.patch against trunk revision 1170886. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/542//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/
        -----------------------------------------------------------

        (Updated 2011-09-14 22:16:44.740700)

        Review request for zookeeper.

        Summary (updated)
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-14 22:16:44.740700) Review request for zookeeper. Summary (updated) -------
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/#review1896
        -----------------------------------------------------------

        Looks good to me with just a couple questions:

        src/java/main/org/apache/zookeeper/server/DataTree.java
        <https://reviews.apache.org/r/1772/#comment4377>

        while you're here, add javadoc?

        src/java/main/org/apache/zookeeper/server/DataTree.java
        <https://reviews.apache.org/r/1772/#comment4378>

        javadoc?

        src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
        <https://reviews.apache.org/r/1772/#comment4379>

        why is this dropped? (I didn't see it noted in the jira)

        • Patrick

        On 2011-09-14 10:12:15, Thomas Koch wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1772/

        -----------------------------------------------------------

        (Updated 2011-09-14 10:12:15)

        Review request for zookeeper.

        Summary

        -------

        .

        This addresses bug ZOOKEEPER-1176.

        https://issues.apache.org/jira/browse/ZOOKEEPER-1176

        Diffs

        -----

        src/java/main/org/apache/zookeeper/server/DataNode.java 9498204

        src/java/main/org/apache/zookeeper/server/DataTree.java 3987c54

        src/java/main/org/apache/zookeeper/server/ZKDatabase.java 6bad5fc

        src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 2946030

        Diff: https://reviews.apache.org/r/1772/diff

        Testing

        -------

        Thanks,

        Thomas

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/#review1896 ----------------------------------------------------------- Looks good to me with just a couple questions: src/java/main/org/apache/zookeeper/server/DataTree.java < https://reviews.apache.org/r/1772/#comment4377 > while you're here, add javadoc? src/java/main/org/apache/zookeeper/server/DataTree.java < https://reviews.apache.org/r/1772/#comment4378 > javadoc? src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java < https://reviews.apache.org/r/1772/#comment4379 > why is this dropped? (I didn't see it noted in the jira) Patrick On 2011-09-14 10:12:15, Thomas Koch wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-14 10:12:15) Review request for zookeeper. Summary ------- . This addresses bug ZOOKEEPER-1176 . https://issues.apache.org/jira/browse/ZOOKEEPER-1176 Diffs ----- src/java/main/org/apache/zookeeper/server/DataNode.java 9498204 src/java/main/org/apache/zookeeper/server/DataTree.java 3987c54 src/java/main/org/apache/zookeeper/server/ZKDatabase.java 6bad5fc src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 2946030 Diff: https://reviews.apache.org/r/1772/diff Testing ------- Thanks, Thomas
        Hide
        Hadoop QA added a comment -

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

        +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/534//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/534//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/534//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/12494417/ZOOKEEPER-1176.patch against trunk revision 1170458. +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/534//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/534//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/534//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/
        -----------------------------------------------------------

        (Updated 2011-09-14 10:12:15.541246)

        Review request for zookeeper.

        Summary
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-14 10:12:15.541246) Review request for zookeeper. Summary -------
        Hide
        Thomas Koch added a comment -

        found that DataTree.clean() is never called, so removed the method.

        Show
        Thomas Koch added a comment - found that DataTree.clean() is never called, so removed the method.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/
        -----------------------------------------------------------

        (Updated 2011-09-14 07:23:57.897945)

        Review request for zookeeper.

        Summary
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- (Updated 2011-09-14 07:23:57.897945) Review request for zookeeper. Summary -------
        Hide
        Thomas Koch added a comment -

        added a "synchronized" to org.apache.zookeeper.server.DataNode.getApproximateDataSize()

        Show
        Thomas Koch added a comment - added a "synchronized" to org.apache.zookeeper.server.DataNode.getApproximateDataSize()
        Hide
        Patrick Hunt added a comment -

        Let's cleanup the findbugs issue.

        Show
        Patrick Hunt added a comment - Let's cleanup the findbugs issue.
        Hide
        Hadoop QA added a comment -

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

        +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 appears to introduce 1 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/520//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/520//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/520//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/12493793/ZOOKEEPER-1176.patch against trunk revision 1166970. +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 appears to introduce 1 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/520//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/520//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/520//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1772/
        -----------------------------------------------------------

        Review request for zookeeper.

        Summary
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1772/ ----------------------------------------------------------- Review request for zookeeper. Summary -------

          People

          • Assignee:
            Thomas Koch
            Reporter:
            Thomas Koch
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development