Hi Colin, Comments inline.
On Tue, May 9, 2017 at 10:37 PM, Colin McCabe <cmcc...@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
