ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1407

Support GetData and GetChildren in Multi

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: None
    • Labels:
      None

      Description

      There is use case where GetData and GetChildren would participate in Multi.
      We should add support for this case.

      1. 1407.txt
        8 kB
        Ted Yu
      2. 1407-v2.txt
        12 kB
        Ted Yu
      3. 1407-v3.txt
        12 kB
        Ted Yu
      4. 1407-v4.txt
        18 kB
        Ted Yu
      5. 1407-v5.txt
        19 kB
        Ted Yu

        Activity

        Hide
        Ted Yu added a comment -

        Initial attempt.

        Show
        Ted Yu added a comment - Initial attempt.
        Hide
        Ted Yu added a comment -
        Show
        Ted Yu added a comment - https://reviews.apache.org/r/4264/ has been created.
        Hide
        Ted Yu added a comment -

        I see SetDataTxn used in a private method, proposeSetData(), of Zab1_0Test.
        w.r.t. writing test for GetData, I would seek some advice.

        Show
        Ted Yu added a comment - I see SetDataTxn used in a private method, proposeSetData(), of Zab1_0Test. w.r.t. writing test for GetData, I would seek some advice.
        Hide
        Ted Yu added a comment -

        Uploaded new version of patch which covers GetChildren.

        I think we should produce better hash code compared to what we have now:

                public int hashCode() {
                    return getType() + getPath().hashCode() + Arrays.hashCode(data) + version;
                }
        
        Show
        Ted Yu added a comment - Uploaded new version of patch which covers GetChildren. I think we should produce better hash code compared to what we have now: public int hashCode() { return getType() + getPath().hashCode() + Arrays.hashCode(data) + version; }
        Hide
        Marshall McMullen added a comment -

        I don't see a new version of the patch to cover GetChildren... did you forget to upload new patch?

        I agree hashCode could be improved.

        Show
        Marshall McMullen added a comment - I don't see a new version of the patch to cover GetChildren... did you forget to upload new patch? I agree hashCode could be improved.
        Hide
        Marshall McMullen added a comment -

        I see the new updated patch in the reviewboard but should upload it to this case as well.

        Show
        Marshall McMullen added a comment - I see the new updated patch in the reviewboard but should upload it to this case as well.
        Hide
        Camille Fournier added a comment -

        Zhihong, it's good if you change the state to "Patch Available" when you've got something for us to look at. We generally look at the patch available queue to determine what we need to review, etc. It will also trigger the automated build check. I've set this one to patch available.

        Show
        Camille Fournier added a comment - Zhihong, it's good if you change the state to "Patch Available" when you've got something for us to look at. We generally look at the patch available queue to determine what we need to review, etc. It will also trigger the automated build check. I've set this one to patch available.
        Hide
        Ted Yu added a comment -

        Thanks Camille.

        Without proper unit tests Ted Dunning requested, I didn't regard the patch to be available for full-scale review.

        Pardon my laziness.

        Show
        Ted Yu added a comment - Thanks Camille. Without proper unit tests Ted Dunning requested, I didn't regard the patch to be available for full-scale review. Pardon my laziness.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517718/1407-v2.txt
        against trunk revision 1302736.

        +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 patch appears to cause tar ant target to fail.

        -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

        +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/1002//testReport/
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1002//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/12517718/1407-v2.txt against trunk revision 1302736. +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 patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +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/1002//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1002//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        I see the following in build output:

             [exec]     [javac] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/main/org/apache/zookeeper/Op.java:293: org.apache.zookeeper.Op.GetChildren is not abstract and does not override abstract method withChroot(java.lang.String) in org.apache.zookeeper.Op
             [exec]     [javac]     public static class GetChildren extends Op {
        

        Looking at other classes (such as Delete) in Op.java, I don't see them having withChroot() method.

        I searched codebase and didn't find where withChroot() is defined.

        For the following:

             [exec]     [javac] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java:758: cannot find symbol
             [exec]     [javac] symbol  : variable debug
             [exec]     [javac] location: class org.apache.zookeeper.server.DataTree
             [exec]     [javac]                   debug = "Get children transaction for "
        

        debug is defined at line 732, outside the try loop.

        Show
        Ted Yu added a comment - I see the following in build output: [exec] [javac] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/main/org/apache/zookeeper/Op.java:293: org.apache.zookeeper.Op.GetChildren is not abstract and does not override abstract method withChroot(java.lang. String ) in org.apache.zookeeper.Op [exec] [javac] public static class GetChildren extends Op { Looking at other classes (such as Delete) in Op.java, I don't see them having withChroot() method. I searched codebase and didn't find where withChroot() is defined. For the following: [exec] [javac] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java:758: cannot find symbol [exec] [javac] symbol : variable debug [exec] [javac] location: class org.apache.zookeeper.server.DataTree [exec] [javac] debug = "Get children transaction for " debug is defined at line 732, outside the try loop.
        Hide
        Marshall McMullen added a comment -

        This is probably related to changes that just went in for https://issues.apache.org/jira/browse/ZOOKEEPER-1344

        Show
        Marshall McMullen added a comment - This is probably related to changes that just went in for https://issues.apache.org/jira/browse/ZOOKEEPER-1344
        Hide
        Ted Yu added a comment -

        Rebased patch based on current trunk.

        Show
        Ted Yu added a comment - Rebased patch based on current 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/12519238/1407-v3.txt
        against trunk revision 1302736.

        +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/1003//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1003//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1003//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/12519238/1407-v3.txt against trunk revision 1302736. +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/1003//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1003//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1003//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        When adding ops.add(Op.getData()) call to MultiTransactionTest.java, I found a lot of places where getData was handled in patch v3.

        I am not sure about the following code in MultiResponse.serialize():

                          Stat stat = new Stat();
                          new GetDataResponse(gdr.getData(), stat).serialize(archive, tag);
                          break;
        
        Show
        Ted Yu added a comment - When adding ops.add(Op.getData()) call to MultiTransactionTest.java, I found a lot of places where getData was handled in patch v3. I am not sure about the following code in MultiResponse.serialize(): Stat stat = new Stat(); new GetDataResponse(gdr.getData(), stat).serialize(archive, tag); break ;
        Hide
        Ted Yu added a comment -

        Found a typo above: should read "where getData wasn't handled in patch v3"

        Show
        Ted Yu added a comment - Found a typo above: should read "where getData wasn't handled in patch v3"
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12520560/1407-v4.txt
        against trunk revision 1307191.

        +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 generated 6 javac compiler warnings (more than the trunk's current 5 warnings).

        -1 findbugs. The patch appears to introduce 3 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/1020//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1020//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1020//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/12520560/1407-v4.txt against trunk revision 1307191. +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 generated 6 javac compiler warnings (more than the trunk's current 5 warnings). -1 findbugs. The patch appears to introduce 3 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/1020//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1020//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1020//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        With 1407-v5.txt, I see the following in test output:

        2012-03-30 10:31:56,090 [myid:] - WARN  [main-SendThread(localhost:11221):ClientCnxn$SendThread@1061] - Session 0x13664a9dee20001 for server localhost/127.0.0.1:11221, unexpected error, closing socket connection and attempting reconnect
        java.io.IOException: Invalid type 256 in MultiResponse
          at org.apache.zookeeper.MultiResponse.deserialize(MultiResponse.java:135)
          at org.apache.zookeeper.ClientCnxn$SendThread.readResponse(ClientCnxn.java:812)
          at org.apache.zookeeper.ClientCnxnSocketNIO.doIO(ClientCnxnSocketNIO.java:94)
          at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:287)
          at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1039)
        
        Show
        Ted Yu added a comment - With 1407-v5.txt, I see the following in test output: 2012-03-30 10:31:56,090 [myid:] - WARN [main-SendThread(localhost:11221):ClientCnxn$SendThread@1061] - Session 0x13664a9dee20001 for server localhost/127.0.0.1:11221, unexpected error, closing socket connection and attempting reconnect java.io.IOException: Invalid type 256 in MultiResponse at org.apache.zookeeper.MultiResponse.deserialize(MultiResponse.java:135) at org.apache.zookeeper.ClientCnxn$SendThread.readResponse(ClientCnxn.java:812) at org.apache.zookeeper.ClientCnxnSocketNIO.doIO(ClientCnxnSocketNIO.java:94) at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:287) at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1039)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12520633/1407-v5.txt
        against trunk revision 1307191.

        +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 3 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/1021//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1021//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1021//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/12520633/1407-v5.txt against trunk revision 1307191. +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 3 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/1021//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1021//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1021//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        cancelling while the findbugs issues are addressed.

        Show
        Patrick Hunt added a comment - cancelling while the findbugs issues are addressed.

          People

          • Assignee:
            Ted Yu
            Reporter:
            Ted Yu
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development