Ashish, +1 from me. The KIP title is mis-leading now since it's no just about principal type. Could we change that?
Thanks, Jun On Tue, Apr 5, 2016 at 1:07 PM, Ashish Singh <asi...@cloudera.com> wrote: > Jun, > > KIP-50 is now updated. Mind taking a look. > > On Mon, Apr 4, 2016 at 10:40 PM, Ashish Singh <asi...@cloudera.com> wrote: > > > Jun, > > > > Your suggested approach works, will update the KIP and re-initiate > voting. > > Thanks! > > > > On Sun, Apr 3, 2016 at 8:37 AM, Jun Rao <j...@confluent.io> wrote: > > > >> Ashish, > >> > >> For the two benefits that you listed, fail fast can be done just in the > >> implementation w/o getSupportedPrincipalTypes(). For avoiding the same > >> check in all implementations, I think Grant is thinking of adding some > ACL > >> RPC request/response btw the client and broker directly, instead of > >> writing > >> the ACL to ZK directly in the future. If we do that, the right place to > do > >> any sanity check is probably in each implementation instead of in > >> AclCommand since AclCommand won't necessarily be the only entry point > for > >> changing ACLs. So, I am still not quite convinced that we should > >> getSupportedPrincipalTypes() > >> right now. It's easy to add a new method to the interface, but hard to > >> remove/change an existing method. Grant, could you comment on the > latter? > >> > >> Thanks, > >> > >> Jun > >> > >> On Sat, Apr 2, 2016 at 11:43 AM, Ashish Singh <asi...@cloudera.com> > >> wrote: > >> > >> > Hello Jun, > >> > > >> > > >> > On Fri, Apr 1, 2016 at 9:57 PM, Jun Rao <j...@confluent.io> wrote: > >> > > >> > > Ashish, > >> > > > >> > > Thanks for the KIP. > >> > > > >> > > It seems that a specific implementation of Authorizer can reject > >> invalid > >> > > user type in addAcl/removeAcl without needing the new > >> > > getSupportedPrincipalTypes() > >> > > method, right? It's probably useful to provide the supported user > >> types > >> > as > >> > > information through CLI (e.g., when --help is specified). Then, > there > >> may > >> > > other information that a specific authorizer may want to provide. > So, > >> if > >> > > this is just informational, would it be better to add sth like > >> > > getDescription() in the Authorizer interface and expose that through > >> CLI? > >> > > > >> > Providing information is definitely an important reason, some other > >> reasons > >> > were to fail fast and to avoid same check in all implementations. I > >> agree > >> > having a generic getDescription() will be handy for authorizer > >> > implementations to provide more implementation specific info, > including > >> > supported principal types and more. However, do you think other two > >> reasons > >> > I mentioned can convince you for current proposal? > >> > > >> > > > >> > > Jun > >> > > > >> > > On Wed, Mar 30, 2016 at 11:02 AM, Ashish Singh <asi...@cloudera.com > > > >> > > wrote: > >> > > > >> > > > Hi Guys, > >> > > > > >> > > > I would like to open the vote for KIP-50. > >> > > > > >> > > > KIP: > >> > > > > >> > > > > >> > > > >> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-50+-+Enhance+Authorizer+interface+to+be+aware+of+supported+Principal+Types > >> > > > > >> > > > Discuss thread: here > >> > > > < > >> > > > > >> > > > >> > > >> > http://mail-archives.apache.org/mod_mbox/kafka-dev/201603.mbox/%3CCAGQG9cUCLDO0owdziDcL9iStXNF1wURyVNcEZedQJg%3DUuC7j%3DQ%40mail.gmail.com%3E > >> > > > > > >> > > > > >> > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-3186 > >> > > > > >> > > > PR: https://github.com/apache/kafka/pull/861 > >> > > > > >> > > > -- > >> > > > > >> > > > Regards, > >> > > > Ashish > >> > > > > >> > > > >> > > >> > > >> > > >> > -- > >> > > >> > Regards, > >> > Ashish > >> > > >> > > > > > > > > -- > > > > Regards, > > Ashish > > > > > > -- > > Regards, > Ashish >
