Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.3.1, 3.3.2, 3.3.3
    • Fix Version/s: 3.5.0
    • Component/s: java client
    • Labels:
      None
    • Tags:
      acl-check

      Description

      I watched the source of the zookeeper class and I missed an acl check in the asynchronous version of the create operation. Is there any reason, that in the asynch version is no
      check whether the acl is valid, or did someone forget to implement it. It's interesting because we worked on a refactoring of the zookeeper client and don't want to implement a bug.

      The following code is missing:
      if (acl != null && acl.size() == 0)

      { throw new KeeperException.InvalidACLException(); }

        Activity

        Hide
        Mahadev konar added a comment -

        Moving this out to later, since this is a change in API (backward incompatible).

        Show
        Mahadev konar added a comment - Moving this out to later, since this is a change in API (backward incompatible).
        Hide
        Patrick Hunt added a comment -

        I don't think we should commit this patch as it currently stands for 3.x. We don't break api b/w compatibility outside of a major version change.

        Can we add this as "create2(...)throws .." (idea for a better name?) and leave create() as-is?

        We'd also need to be explicit in the docs for create and create2 as to why create2 exists and when to use it.

        It does seem like we should deprecate create(), although that's going to generate alot of deprecation warnings...

        Show
        Patrick Hunt added a comment - I don't think we should commit this patch as it currently stands for 3.x. We don't break api b/w compatibility outside of a major version change. Can we add this as "create2(...)throws .." (idea for a better name?) and leave create() as-is? We'd also need to be explicit in the docs for create and create2 as to why create2 exists and when to use it. It does seem like we should deprecate create(), although that's going to generate alot of deprecation warnings...
        Hide
        Benjamin Reed added a comment -

        i'm also worried about changing the signature. i think this is an optimization to save the server from detecting the problem. its not just about the change, but none of the async methods throw exceptions. in reality, if we want to detect this at the client, we need to invoke the callback, which is a bit tricky due to threading issues. i think we should just allow the server to detect it as it currently happens.

        Show
        Benjamin Reed added a comment - i'm also worried about changing the signature. i think this is an optimization to save the server from detecting the problem. its not just about the change, but none of the async methods throw exceptions. in reality, if we want to detect this at the client, we need to invoke the callback, which is a bit tricky due to threading issues. i think we should just allow the server to detect it as it currently happens.
        Hide
        Mahadev konar added a comment -

        Shouldn't this be done for setAcl as well? I am a little worried that this changes the api signatures in 3.4. I dont see this as a very urgent fix to break compatibility just for this corner case. Others?

        Show
        Mahadev konar added a comment - Shouldn't this be done for setAcl as well? I am a little worried that this changes the api signatures in 3.4. I dont see this as a very urgent fix to break compatibility just for this corner case. Others?
        Hide
        Hadoop QA added a comment -

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

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

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

        Trivial patch to fix the issue.

        Show
        Laxman added a comment - Trivial patch to fix the issue.
        Hide
        Thomas Koch added a comment -

        s/ack/acl/, fixed spelling, added code snippet in question

        Show
        Thomas Koch added a comment - s/ack/acl/, fixed spelling, added code snippet in question

          People

          • Assignee:
            Unassigned
            Reporter:
            Patrick Datko
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development