Skip to content

HOTFIX: Use a true sentinel for UseDefaultAcls#2829

Closed
ijuma wants to merge 3 commits intoapache:trunkfrom
ijuma:zk-utils-default-acls-improvement
Closed

HOTFIX: Use a true sentinel for UseDefaultAcls#2829
ijuma wants to merge 3 commits intoapache:trunkfrom
ijuma:zk-utils-default-acls-improvement

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Apr 8, 2017

In 67fc2a9, we are using an empty collection and comparing via
value equality, so if a user passes an empty collection, they will
get the default ACLs instead of no ACLs. We fix that issue here.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Apr 8, 2017

cc @rajinisivaram @gwenshap

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 8, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2840/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 8, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2840/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 8, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2844/
Test PASSed (JDK 8 and Scala 2.11).

@rajinisivaram
Copy link
Copy Markdown
Contributor

@ijuma Thanks for the PR. LGTM.

@gwenshap
Copy link
Copy Markdown
Contributor

gwenshap commented Apr 8, 2017

Just to clarify - we need to merge this into both trunk and 0.10.2 and roll out a new RC?
If we need a new RC, can you please comment on the email vote thread?

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Apr 10, 2017

I looked into this some more and I think we don't need to do another RC. The reason is that passing an empty list of ACLs throws an error in ZkClient.create():

if (acl == null || acl.size() == 0) {
  throw new NullPointerException("Missing value for ACL");
}

So 67fc2a9 only affects the behaviour of code that caused an exception previously. It seems that, in the worst case, it may cause test code that was checking this exceptional condition to break. Given that, it doesn't seem worth another RC by itself.

The next question is whether this PR should be merged at all. It seems safer to throw an exception in case an empty list is passed than to assume the caller wanted the default ACLs to be applied. So, I'd say we should probably merge to trunk and 0.10.2 so that we have consistent behaviour in both branches. If another RC for 0.10.2.1 is required, then we'll include it there, but otherwise, we won't.

Thoughts?

@gwenshap
Copy link
Copy Markdown
Contributor

Sounds good. I'll merge to both branches

asfgit pushed a commit that referenced this pull request Apr 10, 2017
In 67fc2a9, we are using an empty collection and comparing via
value equality, so if a user passes an empty collection, they will
get the default ACLs instead of no ACLs. We fix that issue here.

Author: Ismael Juma <ismael@juma.me.uk>

Reviewers: Rajini Sivaram

Closes #2829 from ijuma/zk-utils-default-acls-improvement and squashes the following commits:

0846172 [Ismael Juma] Add missing import
2dc84f3 [Ismael Juma] Simplify logic in `sensitivePath`
8122f27 [Ismael Juma] Use a true sentinel instead of an empty collection for `UseDefaultAcls`

(cherry picked from commit c31958e)
Signed-off-by: Gwen Shapira <cshapi@gmail.com>
@asfgit asfgit closed this in c31958e Apr 10, 2017
@ijuma ijuma deleted the zk-utils-default-acls-improvement branch September 5, 2017 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants