Thanks Colin. A few quick comments: 1. Is there a reason why AddAclsRequest must be sent to the controller broker? When this was discussed previously, Jun suggested that such a restriction may not be necessary for this request.
2. Other protocol APIs use the Delete prefix instead of Remove. Is there a reason to deviate here? If there's a good reason to do so, we have to fix the one mention of DeleteAcls in the proposal. 3. Do you mean "null" in the following sentence? "Note that an argument of "none" is different than a wildcard argument." 4. How will the non-zero top-level error code be computed? A bit more detail on this would be helpful. As a general rule, most batch protocol APIs don't have a top level error code because errors are usually at the batch element level. This has proved to be a bit of an issue in cases where we want to return a generic error code (e.g. InvalidProtocolVersion). Also, V2 of OffsetFetch has a top-level error code[1]. However, the OffsetFetch behaviour is that we either use the top-level error code or the partition level error codes, which is different than what is being suggested here. 5. Nit: In other requests we used `error_message` instead of `error_string`. 6. Regarding the migration plan, it's worth making it clear that the CLI transition is not part of this KIP. 7. Regarding the forward compatibility point, we could potentially use enums with UNKNOWN element? This pattern has been used elsewhere in Kafka and it would be good to compare it with the proposed solution. An important question is what can users do with UNKNOWN elements. If the assumption is that users ignore them, then it seems like the enum approach may be good enough. Thanks, Ismael [1] https://cwiki.apache.org/confluence/display/KAFKA/KIP-88%3A+OffsetFetch+Protocol+Update On Fri, Apr 21, 2017 at 9:27 PM, Colin McCabe <cmcc...@apache.org> wrote: > Hi all, > > As part of the AdminClient work, we would like to add methods for > adding, deleting, and listing access control lists (ACLs). I wrote up a > KIP to discuss implementing requests for those operations, as well as > AdminClient APIs. Take a look at: > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 140%3A+Add+administrative+RPCs+for+adding%2C+deleting%2C+and+listing+ACLs > > regards, > Colin >
