kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ismael Juma <ism...@juma.me.uk>
Subject Re: [VOTE] KIP-140: Add administrative RPCs for adding, deleting, and listing ACLs
Date Wed, 10 May 2017 15:21:53 GMT
Hi Colin,

Comments inline.

On Tue, May 9, 2017 at 10:37 PM, Colin McCabe <cmccabe@apache.org> wrote:

> On Tue, May 9, 2017, at 08:00, Ismael Juma wrote:
> Good question.  I guess the answer is that I couldn't come up with any
> per-element error codes in DescribeAclsResponse.


Thanks for the explanation.

>
> > 2. CreateAclsResponse don't include the resource type and resource name
> > (the order of the responses has to be used to match against the request).
> > It seems like this would make it harder to debug and it's inconsistent
> > with other responses. Is it really worth the space savings?
>
> I found it helpful to prototype this, and the code is up on the
> KAFKA-3266 pull request.  I didn't find it harder to debug due to the
> dependency on request ordering.  It was a little bit harder to write
> since I had to preserve the request ordering, but not that bad.
>

I

As to whether this is inconsistent with other responses... I think
> that's debatable.  We don't send back the text of the messages which
> producers send to us when acknowledging it.  CreateTopicsResponse
> doesn't include the full topic creation request-- it just includes the
> topic names.


Yes, I meant something along these lines. Basically a map from resource to
the created ACEs for that resource. More below.


> We couldn't do the same thing with CreateAclsResponse or
> DeleteAclsResponse simply because there is no "name" by which an
> individual ACL can be identified.  Including the entire request in the
> response feels like a very heavy hammer to me.
>

I agree that including the entire request in the response is not
desireable. And given the issue with uniquely identifying an ACE, the
approach that is currently proposed is probably OK.

A couple of minor suggestions that we discussed offline as well:

1. Rename AclData to AccessControlEntry (or AclEntry)
2. Rename AclFixture to AclBinding. That eliminates the chance that people
may confuse it with a Test fixture for Acl (
https://en.wikipedia.org/wiki/Test_fixture).

Ismael

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message