Hi Jun, Thanks for the review.
On Thu, Apr 27, 2017, at 14:21, Jun Rao wrote: > 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. That's a fair point. I'll change it so that we can send these to any broker. > > 10. Since we may add new operations in the future, perhaps use 0 for the > ALL operation? OK. > > 11. "That is, ListAclsRequest(principal=none) will return all ACLs". Do > you > mean principal == null? Right, this meant principal == null. I'll make that a little more explicit. > > 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? Yeah, let's use ClusterAction here. best, Colin > > 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 > > > > > >
