ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1344

ZooKeeper client multi-update command is not considering the Chroot request

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 3.4.0
    • Fix Version/s: 3.4.4, 3.5.0
    • Component/s: java client
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change

      Description

      For example:
      I have created a ZooKeeper client with subtree as "10.18.52.144:2179/apps/X". Now just generated OP command for the creation of zNode "/myId". When the client creates the path "/myid", the ZooKeeper server is actually be creating the path as "/myid" instead of creating as "/apps/X/myid"

      Expected output: zNode has to be created as "/apps/X/myid"

      1. ZOOKEEPER-1344-onlytestcase.patch
        2 kB
        Rakesh R
      2. ZOOKEEPER-1344.patch
        11 kB
        Rakesh R
      3. ZOOKEEPER-1344.1.patch
        11 kB
        Rakesh R
      4. ZOOKEEPER-1344_trunk.patch
        11 kB
        Rakesh R
      5. ZOOKEEPER-1344_branch_3.4.patch
        11 kB
        Rakesh R

        Activity

        Hide
        Rakesh R added a comment -

        I have attached patch contains only the test case to re-produce the scenario. Could you please review the behaviour.

        Show
        Rakesh R added a comment - I have attached patch contains only the test case to re-produce the scenario. Could you please review the behaviour.
        Hide
        Hadoop QA added a comment -

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

        +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/859//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/859//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/859//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/12508678/ZOOKEEPER-1344.patch against trunk revision 1222816. +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/859//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/859//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/859//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        First approach could be, expose Op.setPath(path) method. On invocation of multi(ops) api, will iterate and modify directly to the client Op objects by prepending chRoot. I feel, this appraoch will break the immutability of Op and is modifying the Op passed by the client. Hence it will not able to reuse Op objects on a non-chRoot clients.

        Second approach could be, provide Op.clone(String path) method. On invocation of multi(ops) api, will iterate and create a new Op object by prepending chRoot path, and retains Op client object as immutable. Made clone(path) as abstract and will be overidden for new operation type. IMO, the second one could be the better way.

        I have attached the patch based on the second appraoch, please review the changes and test cases.

        Case: Also I have seen the 'path' validation is missing for the multi(ops) api. IMO, we can raise a new task/subtask and address the issue separately, since it involves method signature modification of Op apis by throwing 'KeeperException'.
        If everyone agrees, I would like to help by doing the changes.

        Thanks
        Rakesh

        Show
        Rakesh R added a comment - First approach could be, expose Op.setPath(path) method. On invocation of multi(ops) api, will iterate and modify directly to the client Op objects by prepending chRoot. I feel, this appraoch will break the immutability of Op and is modifying the Op passed by the client. Hence it will not able to reuse Op objects on a non-chRoot clients. Second approach could be, provide Op.clone(String path) method. On invocation of multi(ops) api, will iterate and create a new Op object by prepending chRoot path, and retains Op client object as immutable. Made clone(path) as abstract and will be overidden for new operation type. IMO, the second one could be the better way. I have attached the patch based on the second appraoch, please review the changes and test cases. Case: Also I have seen the 'path' validation is missing for the multi(ops) api. IMO, we can raise a new task/subtask and address the issue separately, since it involves method signature modification of Op apis by throwing 'KeeperException'. If everyone agrees, I would like to help by doing the changes. Thanks Rakesh
        Hide
        Patrick Hunt added a comment -

        Marshall & Ted - could you provide some review feedback for this patch? Thanks.

        Show
        Patrick Hunt added a comment - Marshall & Ted - could you provide some review feedback for this patch? Thanks.
        Hide
        Ted Dunning added a comment -

        I don't like the use of the name clone here. This is not a clone at all.

        An alternative might be to call it copyWithNewPath or reroot or withPath. Of these, withPath strikes my fancy a bit.

        Otherwise, this change seems benign and an excellent idea.

        Show
        Ted Dunning added a comment - I don't like the use of the name clone here. This is not a clone at all. An alternative might be to call it copyWithNewPath or reroot or withPath. Of these, withPath strikes my fancy a bit. Otherwise, this change seems benign and an excellent idea.
        Hide
        Marshall McMullen added a comment -

        I agree with Ted, I don't like the name Clone either. It's just a copy with a new chroot prefix... Ted's suggestions are good. I like CopyWithPath better than CopyWithNewPath as the New is redundant. Alternatively maybe CopyWithChroot to better indicate it's purpose.

        The rest of the patch and the unit tests look good to me. Excellent idea by the way.

        Show
        Marshall McMullen added a comment - I agree with Ted, I don't like the name Clone either. It's just a copy with a new chroot prefix... Ted's suggestions are good. I like CopyWithPath better than CopyWithNewPath as the New is redundant. Alternatively maybe CopyWithChroot to better indicate it's purpose. The rest of the patch and the unit tests look good to me. Excellent idea by the way.
        Hide
        Ted Dunning added a comment -

        On reading the code, it really seems that even the copy verb is redundant. For instance, this
        looks good:

                for (Op op : ops) {
                   transaction.add(op.withChroot(addRootPrefix(op.getPath)));
                }
        
        Show
        Ted Dunning added a comment - On reading the code, it really seems that even the copy verb is redundant. For instance, this looks good: for (Op op : ops) { transaction.add(op.withChroot(addRootPrefix(op.getPath))); }
        Hide
        Rakesh R added a comment -

        Thanks for the interest and comments. I have uploaded the latest patch includes fix. Please have a look.

        Also I have seen the 'path validation' is missing for the multi(ops) api. IMO, would raise a new task/subtask and address the issue separately, since it involves method signature modification of Op apis by throwing 'KeeperException' (affects backward compatibility) ?

        -Rakesh

        Show
        Rakesh R added a comment - Thanks for the interest and comments. I have uploaded the latest patch includes fix. Please have a look. Also I have seen the 'path validation' is missing for the multi(ops) api. IMO, would raise a new task/subtask and address the issue separately, since it involves method signature modification of Op apis by throwing 'KeeperException' (affects backward compatibility) ? -Rakesh
        Hide
        Hadoop QA added a comment -

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

        +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/885//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/885//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/885//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/12509678/ZOOKEEPER-1344.1.patch against trunk revision 1227927. +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/885//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/885//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/885//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        @rakesh - certainly, please file a separate jira for that oversight (path validation)

        Show
        Patrick Hunt added a comment - @rakesh - certainly, please file a separate jira for that oversight (path validation)
        Hide
        Rakesh R added a comment -

        Thanks Pat for looking into the issue, I have raised 'ZOOKEEPER-1388' for discussing the zk client side 'path validation'.

        Show
        Rakesh R added a comment - Thanks Pat for looking into the issue, I have raised ' ZOOKEEPER-1388 ' for discussing the zk client side 'path validation'.
        Hide
        Benjamin Reed added a comment -

        pat and marshall, is this patch ready to go?

        Show
        Benjamin Reed added a comment - pat and marshall, is this patch ready to go?
        Hide
        Marshall McMullen added a comment -

        Yep, looks good to me.

        Show
        Marshall McMullen added a comment - Yep, looks good to me.
        Hide
        Camille Fournier added a comment -

        I hate to say it but this patch no longer applies. Can you please regenerate so that it applies to latest trunk and 3.4 branch if necessary, so we can check it in? Thanks.

        Show
        Camille Fournier added a comment - I hate to say it but this patch no longer applies. Can you please regenerate so that it applies to latest trunk and 3.4 branch if necessary, so we can check it in? Thanks.
        Hide
        Rakesh R added a comment -

        I have attached the patch for the branch 3.4. I will soon upload the patch for the trunk version.

        Show
        Rakesh R added a comment - I have attached the patch for the branch 3.4. I will soon upload the patch for the trunk version.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517748/ZOOKEEPER-1344_branch_3.4.patch
        against trunk revision 1297740.

        +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/982//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/982//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/982//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/12517748/ZOOKEEPER-1344_branch_3.4.patch against trunk revision 1297740. +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/982//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/982//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/982//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        Attached patch which is created in trunk version. Please do review and help me to commit the changes. Thanks

        Show
        Rakesh R added a comment - Attached patch which is created in trunk version. Please do review and help me to commit the changes. Thanks
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518195/ZOOKEEPER-1344_trunk.patch
        against trunk revision 1297740.

        +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/992//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/992//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/992//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/12518195/ZOOKEEPER-1344_trunk.patch against trunk revision 1297740. +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/992//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/992//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/992//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Thanks Rakesh!

        Show
        Patrick Hunt added a comment - Thanks Rakesh!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1494 (See https://builds.apache.org/job/ZooKeeper-trunk/1494/)
        ZOOKEEPER-1344. ZooKeeper client multi-update command is not considering the Chroot request (Rakesh R via phunt) (Revision 1301837)

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

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Transaction.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1494 (See https://builds.apache.org/job/ZooKeeper-trunk/1494/ ) ZOOKEEPER-1344 . ZooKeeper client multi-update command is not considering the Chroot request (Rakesh R via phunt) (Revision 1301837) Result = SUCCESS phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1301837 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/Transaction.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
        Hide
        Rakesh R added a comment -

        Thanks Pat for the review and committing the changes

        Show
        Rakesh R added a comment - Thanks Pat for the review and committing the changes
        Hide
        Patrick Hunt added a comment -

        No thanks necessary. Thank you for sticking with it. Kudos.

        Show
        Patrick Hunt added a comment - No thanks necessary. Thank you for sticking with it. Kudos.
        Hide
        Patrick Hunt added a comment -

        No thanks necessary. Thank you for sticking with it. Kudos.

        Show
        Patrick Hunt added a comment - No thanks necessary. Thank you for sticking with it. Kudos.

          People

          • Assignee:
            Rakesh R
            Reporter:
            Rakesh R
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development