Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.3.1
    • Fix Version/s: 3.4.0
    • Component/s: java client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      atomic operations

      Description

      DeleteRecursive has been committed to trunk already as a method to the
      zookeeper class. So in the API it has the same level as the atomic operations
      create, delete, getData, setData, etc. The user must get the false impression,
      that deleteRecursive is also an atomic operation.
      It would be better to have deleteRecursive in some helper class but not that
      deep in zookeeper's core code. Maybe I'd like to have another policy on how to
      react if deleteRecursive fails in the middle of its work?

        Activity

        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1266 (See https://builds.apache.org/job/ZooKeeper-trunk/1266/)
        ZOOKEEPER-839. deleteRecursive does not belong to the other methods. (mahadev)

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

        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/ZooKeeperTest.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZKUtil.java
        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1266 (See https://builds.apache.org/job/ZooKeeper-trunk/1266/ ) ZOOKEEPER-839 . deleteRecursive does not belong to the other methods. (mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1157559 Files : /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/ZooKeeperTest.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZKUtil.java /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
        Hide
        Mahadev konar added a comment -

        I just pushed this. Thanks Patrick for bringing this up!

        Show
        Mahadev konar added a comment - I just pushed this. Thanks Patrick for bringing this up!
        Hide
        Hadoop QA added a comment -

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

        +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/451//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/451//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/451//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/12488858/ZOOKEEPER-839.patch against trunk revision 1156497. +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/451//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/451//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/451//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/12488858/ZOOKEEPER-839.patch
        against trunk revision 1152141.

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

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/440//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/440//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/440//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/12488858/ZOOKEEPER-839.patch against trunk revision 1152141. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/440//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/440//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/440//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/12488858/ZOOKEEPER-839.patch
        against trunk revision 1152141.

        +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/434//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/434//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/434//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/12488858/ZOOKEEPER-839.patch against trunk revision 1152141. +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/434//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/434//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/434//console This message is automatically generated.
        Hide
        Benjamin Reed added a comment -

        +1 i like it mahadev

        Show
        Benjamin Reed added a comment - +1 i like it mahadev
        Hide
        Mahadev konar added a comment -

        patch attached moves the methods to ZKUtil.java.

        Show
        Mahadev konar added a comment - patch attached moves the methods to ZKUtil.java.
        Hide
        Mahadev konar added a comment -

        Ben,
        You mean private or package private? If we are going to make it private I'd rather remove it then make it private. Should we just remove it from ZK and then open a jira for adding it to ZK Util?

        Show
        Mahadev konar added a comment - Ben, You mean private or package private? If we are going to make it private I'd rather remove it then make it private. Should we just remove it from ZK and then open a jira for adding it to ZK Util?
        Hide
        Benjamin Reed added a comment -

        i think the quickest fix for this is to make that method private.

        Show
        Benjamin Reed added a comment - i think the quickest fix for this is to make that method private.
        Hide
        Mahadev konar added a comment - - edited

        Looks like listSubTreeBFS also needs to be removed. Ill upload patch.

        Where should I put these though? A ZKUtil like class?

        Ben/Pat any comments?

        Show
        Mahadev konar added a comment - - edited Looks like listSubTreeBFS also needs to be removed. Ill upload patch. Where should I put these though? A ZKUtil like class? Ben/Pat any comments?
        Hide
        Benjamin Reed added a comment -

        i guess i should clarify that i'm agreeing with patrick datko

        Show
        Benjamin Reed added a comment - i guess i should clarify that i'm agreeing with patrick datko
        Hide
        Benjamin Reed added a comment -

        i agree with patrick. it doesn't have the atomicity behavior of other methods and should be in a helper class.

        Show
        Benjamin Reed added a comment - i agree with patrick. it doesn't have the atomicity behavior of other methods and should be in a helper class.
        Hide
        Patrick Hunt added a comment -

        blocker for 3.4, we don't want to ship this as a client api then change it later.

        Show
        Patrick Hunt added a comment - blocker for 3.4, we don't want to ship this as a client api then change it later.

          People

          • Assignee:
            Mahadev konar
            Reporter:
            Patrick Datko
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development