Hi, Colin, Thanks for the KIP. Looks good overall. Just a few minor comments.
1. The controller is responsible for managing the state of topics. So, it makes sense for create/delete topic requests to be sent to the controller. The controller is not responsible for managing ACLs right now. So, it's a bit weird to send acl requests only to the controller. Sending the acl request to any broker is more intuitive and easier to implement. 10. Since we may add new operations in the future, perhaps use 0 for the ALL operation? 11. "That is, ListAclsRequest(principal=none) will return all ACLs". Do you mean principal == null? 12. "Principals must possess Cluster:All permissions to call CreateAclsRequest". All just means all types of operations. So, we need a specific operation that allows CreateAclsRequest. Will that be ClusterAction or a new one? Jun On Mon, Apr 24, 2017 at 10:57 AM, Colin McCabe <cmcc...@apache.org> wrote: > Thanks for taking a look, Ismael. > > On Mon, Apr 24, 2017, at 04:36, Ismael Juma wrote: > > 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. > > Hmm. I guess my thinking here was that since the controller handles > AddTopics and DeleteTopics requests, it would be nice if it had the most > up-to-date ACL information. This was also in the original KIP-4 > proposal. However, given that auth is ZK (or a pluggable system, > optionally) there is no inherent reason the controller broker has to be > the only one to make the change. What do you think? > > > > > 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. > > Good point. Let's make it consistent by changing it to DeleteAcls. I > will also change AddAclsRequest to CreateAclsRequest to match > CreateTopicsRequest. > > > > > 3. Do you mean "null" in the following sentence? "Note that an argument > > of > > "none" is different than a wildcard argument." > > For the string types, NULL is considered "none"; for the INT8 types, -1 > is considered "none". > > > > > 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. > > The idea behind the top-level error code is to implement error codes > that don't have to do with elements in the batch. For example, if we > implement backpressure, the server could send back an error code of > "slow down" telling the client to resend the request after a few > milliseconds have elapsed. > > As you mention, we ought to have a generic response header that would > let us intelligently handle situations like "server doesn't understand > this request version or type" Maybe this is something that needs to be > handled in another KIP, though, since it would require all the responses > to have this, and in the same format. I guess I will remove this for > now. > > > > > 5. Nit: In other requests we used `error_message` instead of > > `error_string`. > > OK. > > > 6. Regarding the migration plan, it's worth making it clear that the CLI > > transition is not part of this KIP. > > OK. > > > > > 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. > > Hmm. It seemed straightforward to let callers see the numeric value of > the element. It makes the command-line tools more useful when > interacting with clusters that have newer versions of the software, for > example. I guess using UNKNOWN instead of UNKNOWN(<number>) has the > advantage of hiding the internal representation better. > > best, > Colin > > > > > > 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 > > > >
