I think it's great that we're moving the interface to java and fixing some of the naming foibles.
This isn't explicit in the KIP which just refers to the java package name (I think), but it looks like you are proposing adding a new authorizer jar for this new package and adding it as a dependency for the client jar. This is a bit inconsistent with how we decided to package stuff when we started with the new clients so it'd be good to work that out. To date the categorization has been: 1. Anything which is just in the clients is in org.apache.clients under clients/ 2. Anything which is in the server is kafka.* which is under core/ 3. Anything which is needed in both places (as it sounds like some enums for authorization are?) is in common which is under clients/ org.apache.clients and org.apache.common are both pure java and dependency free other than the compression libraries and slf4j and are packaged into the kafka-clients.java, the server has it's own jar which has richer dependencies and depends on the client jar. There are other ways this could have been done--e.g. common could have been its own library or even split into multiple sub-libraries--but the decision at that time was just to keep it simple and hard to mess up. Based on the experience with the scala clients our plan was to be ultra-hostile to any added client dependencies. So I think if we're continuing this model we would put the shared authorizer code somewhere under clients/src/main/java/org/apache/kafka/common as with the other shared authorizer. If we're moving away from this model we should probably rethink things and be consistent with this, at the very least splitting up common and clients. -Jay On Wed, Apr 20, 2016 at 1:04 PM, Ashish Singh <asi...@cloudera.com> wrote: > Jun/ Jay/ Gwen/ Harsha/ Ismael, > > As you guys have provided feedback on this earlier, could you review the > KIP again? I have updated the PR <https://github.com/apache/kafka/pull/861> > as > well. > > On Wed, Apr 20, 2016 at 1:01 PM, Ashish Singh <asi...@cloudera.com> wrote: > > > Hi Grant, > > > > On Tue, Apr 19, 2016 at 8:13 AM, Grant Henke <ghe...@cloudera.com> > wrote: > > > >> Hi Ashish, > >> > >> Thanks for the updates. I have a few questions below: > >> > >> > Move following interfaces to new package, org.apche.kafka.authorizer. > >> > > >> > 1. Authorizer > >> > 2. Acl > >> > 3. Operation > >> > 4. PermissionType > >> > 5. Resource > >> > 6. ResourceType > >> > 7. KafkaPrincipal > >> > 8. Session > >> > > >> > > >> This means the client would be required to depend on the authorizer > >> package > >> as a part of KIP-4. Another option is to have the client objects in > >> common. > >> Have we ruled out leaving the interface in the core module? > >> > > With this entities that use Authorizer will depend only on Authorizer > > package. Third party implementations can have only the authorizer pkg as > > dependency. core and client modules will also have to depend on the > > authorizer with this approach. Do you see any issue with it? > > > >> > >> Authorizer interface will be updated to remove getter naming convention. > >> > >> > >> Now that this is Java do we still want to change to the Scala naming > >> convention? > >> > > Even in clients module I do not see getter naming convention being > > followed, it is better to be consistent I guess. > > > >> > >> > >> Since we are completely rewriting the interface, can we add some (at > least > >> one to start with) standard exceptions that each method is recommended > to > >> use/throw? This will help the server in KIP-4 provide meaningful error > >> codes. KAFKA-3507 <https://issues.apache.org/jira/browse/KAFKA-3507> is > >> tracking it right now. > >> > > That should be good to have. Will include that. Thanks. > > > >> > >> Thanks, > >> Grant > >> > >> > >> > >> On Tue, Apr 19, 2016 at 9:48 AM, Ashish Singh <asi...@cloudera.com> > >> wrote: > >> > >> > I have updated KIP-50 > >> > < > >> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-50+-+Move+Authorizer+to+a+separate+package > >> > > > >> > and PR <https://github.com/apache/kafka/pull/861> as per recent > >> > discussions. Please take a look. > >> > > >> > @Harsha / Don, it would be nice if you guys can review the KIP and PR > as > >> > well. > >> > > >> > > >> > On Mon, Apr 11, 2016 at 7:36 PM, Ashish Singh <asi...@cloudera.com> > >> wrote: > >> > > >> > > Yes, Jun. I would like to try get option 2 in, if possible in 0.10. > I > >> am > >> > > not asking for delaying 0.10 for it, but some reviews and early > >> feedback > >> > > would be great. At this point this is what I have in mind. > >> > > > >> > > 1. Move authorizer and related entities to its own package. Note > that > >> I > >> > am > >> > > proposing to drop scala interface completely. Ranger team is fine > >> with it > >> > > and I will update Sentry. > >> > > 2. The only new public method that will be added to authorizer > >> interface > >> > > is description(). > >> > > 3. Update SimpleAclAuthorizer to use the new interface and classes. > >> > > > >> > > On Mon, Apr 11, 2016 at 6:38 PM, Jun Rao <j...@confluent.io> wrote: > >> > > > >> > >> Ashish, > >> > >> > >> > >> So, you want to take a shot at option 2 for 0.10.0? That's fine > with > >> me > >> > >> too. I am just not sure if we have enough time to think through the > >> > >> changes. > >> > >> > >> > >> Thanks, > >> > >> > >> > >> Jun > >> > >> > >> > >> On Mon, Apr 11, 2016 at 6:05 PM, Ashish Singh <asi...@cloudera.com > > > >> > >> wrote: > >> > >> > >> > >> > Hello Jun, > >> > >> > > >> > >> > The 3rd option will require Apache Sentry to go GA with current > >> > >> authorizer > >> > >> > interface, and at this point it seems that the interface won't > last > >> > >> long. > >> > >> > Within a few months, Sentry will have to make a breaking change. > I > >> do > >> > >> > understand that Kafka should not have to delay its release due to > >> one > >> > of > >> > >> > the authorizer implementations. However, can we assist Sentry > >> users to > >> > >> > avoid that breaking upgrade? I think it is worth a shot. If the > >> > changes > >> > >> are > >> > >> > not done by 0.10 code freeze, then sure lets punt it to next > >> release. > >> > >> Does > >> > >> > this seem reasonable to you? > >> > >> > > >> > >> > On Sun, Apr 10, 2016 at 11:42 AM, Jun Rao <j...@confluent.io> > >> wrote: > >> > >> > > >> > >> > > Ashish, > >> > >> > > > >> > >> > > A 3rd option is to in 0.10.0, just sanity check the principal > >> type > >> > in > >> > >> the > >> > >> > > implementation of addAcls/removeAcls of Authorizer, but don't > >> change > >> > >> the > >> > >> > > Authorizer api to add the getDescription() method. This fixes > the > >> > >> > immediate > >> > >> > > issue that an acl rule with the wrong principal type is > silently > >> > >> ignored. > >> > >> > > Knowing valid user types is nice, but not critical (we can > >> include > >> > the > >> > >> > > supported user type in the UnsupportedPrincipalTypeException > >> thrown > >> > >> from > >> > >> > > addAcls/removeAcls). This will give us more time to clean up > the > >> > >> > Authorizer > >> > >> > > api post 0.10.0. > >> > >> > > > >> > >> > > Thanks > >> > >> > > > >> > >> > > Jun > >> > >> > > > >> > >> > > On Fri, Apr 8, 2016 at 9:04 AM, Ashish Singh < > >> asi...@cloudera.com> > >> > >> > wrote: > >> > >> > > > >> > >> > > > Thanks for the input Don. One of the possible paths for > Option > >> 2 > >> > is > >> > >> to > >> > >> > > > completely drop Scala interface, would that be Ok with you > >> folks? > >> > >> > > > > >> > >> > > > On Thursday, April 7, 2016, Don Bosco Durai < > bo...@apache.org> > >> > >> wrote: > >> > >> > > > > >> > >> > > > > Ranger team would prefer option #2. Right now, we have to > >> access > >> > >> some > >> > >> > > of > >> > >> > > > > the nested constants using constructs like Group$.MODULE$, > >> which > >> > >> is > >> > >> > not > >> > >> > > > > intuitive in Java. > >> > >> > > > > > >> > >> > > > > Thanks > >> > >> > > > > > >> > >> > > > > Bosco > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > On 4/7/16, 4:30 PM, "Ashish Singh" <asi...@cloudera.com > >> > >> > > <javascript:;>> > >> > >> > > > > wrote: > >> > >> > > > > > >> > >> > > > > >Harsha/ Don, > >> > >> > > > > > > >> > >> > > > > >Are you guys OK with option 2? I am not aware of all the > >> > existing > >> > >> > > > > >authorizer implementations, however ranger has one for > sure. > >> > >> Getting > >> > >> > > > > direct > >> > >> > > > > >feedback from you guys will be really valuable. > >> > >> > > > > > > >> > >> > > > > >On Thu, Apr 7, 2016 at 3:52 PM, Ismael Juma < > >> ism...@juma.me.uk > >> > >> > > > > <javascript:;>> wrote: > >> > >> > > > > > > >> > >> > > > > >> Hi Don, > >> > >> > > > > >> > >> > >> > > > > >> This is true in Java 7, but Java 8 introduces default > >> methods > >> > >> and > >> > >> > > this > >> > >> > > > > >> workaround is no longer required. During the Interceptor > >> KIP > >> > >> > > > > discussion, it > >> > >> > > > > >> was decided that it was fine to stick to interfaces > given > >> > that > >> > >> we > >> > >> > > are > >> > >> > > > > >> likely to move to Java 8 in the nearish future (probably > >> no > >> > >> later > >> > >> > > than > >> > >> > > > > the > >> > >> > > > > >> Java 9 release). > >> > >> > > > > >> > >> > >> > > > > >> Ismael > >> > >> > > > > >> > >> > >> > > > > >> On Thu, Apr 7, 2016 at 11:36 PM, Don Bosco Durai < > >> > >> > bo...@apache.org > >> > >> > > > > <javascript:;>> wrote: > >> > >> > > > > >> > >> > >> > > > > >> > Hi Ashish > >> > >> > > > > >> > > >> > >> > > > > >> > If we are going by option #2, then I can suggest we > >> give an > >> > >> > > abstract > >> > >> > > > > >> > implementation of the Interface and recommend anyone > >> > >> > implementing > >> > >> > > > > their > >> > >> > > > > >> own > >> > >> > > > > >> > plugin to extend from the abstract class, rather than > >> > >> implement > >> > >> > > the > >> > >> > > > > >> > interface? > >> > >> > > > > >> > > >> > >> > > > > >> > The advantage is, in the future if we add add any new > >> > >> methods in > >> > >> > > the > >> > >> > > > > >> > Interface (e.g. Similar to getDescription()), then we > >> can > >> > >> give a > >> > >> > > > dummy > >> > >> > > > > >> > implementation of the new method and this won’t break > >> the > >> > >> > > > compilation > >> > >> > > > > of > >> > >> > > > > >> > any external implementation. Else over the time it > will > >> be > >> > >> > > > challenging > >> > >> > > > > >> for > >> > >> > > > > >> > anyone customizing the implementation to keep track of > >> > >> changes > >> > >> > to > >> > >> > > > the > >> > >> > > > > >> > Interface. > >> > >> > > > > >> > > >> > >> > > > > >> > Thanks > >> > >> > > > > >> > > >> > >> > > > > >> > Bosco > >> > >> > > > > >> > > >> > >> > > > > >> > > >> > >> > > > > >> > > >> > >> > > > > >> > > >> > >> > > > > >> > On 4/7/16, 11:21 AM, "Ashish Singh" < > >> asi...@cloudera.com > >> > >> > > > > <javascript:;>> wrote: > >> > >> > > > > >> > > >> > >> > > > > >> > >Hello Harsha, > >> > >> > > > > >> > > > >> > >> > > > > >> > >On Thu, Apr 7, 2016 at 11:03 AM, Harsha < > >> m...@harsha.io > >> > >> > > > > <javascript:;>> wrote: > >> > >> > > > > >> > > > >> > >> > > > > >> > >"My only ask is to have this in 0.10. As Jay pointed > >> out, > >> > >> right > >> > >> > > now > >> > >> > > > > >> > >> there > >> > >> > > > > >> > >> are not many implementations out there, we might > >> want to > >> > >> fix > >> > >> > it > >> > >> > > > > ASAP." > >> > >> > > > > >> > >> > >> > >> > > > > >> > >> Probably there aren't many implementations but > there > >> are > >> > >> lot > >> > >> > of > >> > >> > > > > users > >> > >> > > > > >> > >> using these implementations in production clusters. > >> > >> > > > > >> > >> Isn't this going to break the rolling upgrade? > >> > >> > > > > >> > > > >> > >> > > > > >> > >It will and it is a concern, in my previous mail I > have > >> > >> > mentioned > >> > >> > > > > this > >> > >> > > > > >> as > >> > >> > > > > >> > >an issue if we choose to go this route. However, if > we > >> > >> actually > >> > >> > > > > decide > >> > >> > > > > >> to > >> > >> > > > > >> > >do this, I would say it is better to do it sooner > than > >> > >> later, > >> > >> > as > >> > >> > > > > fewer > >> > >> > > > > >> > >implementations will be affected. Below is excerpt > >> from my > >> > >> > > previous > >> > >> > > > > >> mail. > >> > >> > > > > >> > > > >> > >> > > > > >> > >Increase scope of KIP-50 to move authorizer and > related > >> > >> classes > >> > >> > > to > >> > >> > > > a > >> > >> > > > > >> > >separate package. The new package will have java > >> > interface. > >> > >> > This > >> > >> > > > will > >> > >> > > > > >> > allow > >> > >> > > > > >> > >implementations to not depend on kafka core and just > on > >> > >> > > authorizer > >> > >> > > > > >> > package, > >> > >> > > > > >> > >make authorization interface follow kafka’s coding > >> > standards > >> > >> > and > >> > >> > > > will > >> > >> > > > > >> > allow > >> > >> > > > > >> > >java implementations to be cleaner. We can either > >> > completely > >> > >> > drop > >> > >> > > > > scala > >> > >> > > > > >> > >interface, which might be a pain for existing > >> > >> implementations, > >> > >> > or > >> > >> > > > we > >> > >> > > > > can > >> > >> > > > > >> > >have scala interface wrap java interface. Later > allows > >> a > >> > >> > cleaner > >> > >> > > > > >> > >deprecation path for existing scala authorizer > >> interface, > >> > >> > however > >> > >> > > > it > >> > >> > > > > may > >> > >> > > > > >> > or > >> > >> > > > > >> > >may not be feasible as Kafka server will have to > >> somehow > >> > >> decide > >> > >> > > > which > >> > >> > > > > >> > >interface it should be looking for while loading > >> > authorizer > >> > >> > > > > >> > implementation, > >> > >> > > > > >> > >this can probably be solved with a config or some > >> > >> reflection. > >> > >> > If > >> > >> > > we > >> > >> > > > > >> choose > >> > >> > > > > >> > >to go this route, I can dig deeper. > >> > >> > > > > >> > > > >> > >> > > > > >> > >If we go with option 2 and commit on getting this in > >> ASAP, > >> > >> > > > > preferably in > >> > >> > > > > >> > >0.10, there will be fewer implementations that will > be > >> > >> > affected. > >> > >> > > > > >> > > > >> > >> > > > > >> > >and also moving to Java , > >> > >> > > > > >> > >> a authorizer implementation going to run inside a > >> > >> KafkaBroker > >> > >> > > > and I > >> > >> > > > > >> > >> don't see why this is necessary to move to clients > >> > >> package. > >> > >> > > > > >> > >> Are we planning on introducing common module to > have > >> it > >> > >> > > > > independent of > >> > >> > > > > >> > >> broker and client code? > >> > >> > > > > >> > >> > >> > >> > > > > >> > >Yes, I think that would take away the requirement of > >> > >> depending > >> > >> > on > >> > >> > > > > Kafka > >> > >> > > > > >> > >core from authorizer implementations. Do you suggest > >> > >> otherwise? > >> > >> > > > > >> > > > >> > >> > > > > >> > > > >> > >> > > > > >> > >> -Harsha > >> > >> > > > > >> > >> > >> > >> > > > > >> > >> On Thu, Apr 7, 2016, at 10:52 AM, Ashish Singh > wrote: > >> > >> > > > > >> > >> > We might want to take a call here. Following are > >> the > >> > >> > options. > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > 1. Let KIP-50 be the way it is, i.e., just add > >> > >> > > > > getDescription() > >> > >> > > > > >> to > >> > >> > > > > >> > >> > existing scala authorizer interface. It will > >> break > >> > >> > binary > >> > >> > > > > >> > >> > compatibility > >> > >> > > > > >> > >> > (only when using CLI and/or AdminCommand from > >= > >> > 0.10 > >> > >> > > > against > >> > >> > > > > >> > >> > authorizer > >> > >> > > > > >> > >> > implementations based on 0.9.). If we go this > >> > route, > >> > >> it > >> > >> > > is a > >> > >> > > > > >> > simpler > >> > >> > > > > >> > >> > change > >> > >> > > > > >> > >> > and existing implementations won’t have to > >> change > >> > >> > anything > >> > >> > > > on > >> > >> > > > > >> their > >> > >> > > > > >> > >> > end. > >> > >> > > > > >> > >> > 2. Increase scope of KIP-50 to move authorizer > >> and > >> > >> > related > >> > >> > > > > >> classes > >> > >> > > > > >> > to > >> > >> > > > > >> > >> > a > >> > >> > > > > >> > >> > separate package. The new package will have > java > >> > >> > > interface. > >> > >> > > > > This > >> > >> > > > > >> > will > >> > >> > > > > >> > >> > allow > >> > >> > > > > >> > >> > implementations to not depend on kafka core > and > >> > just > >> > >> on > >> > >> > > > > >> authorizer > >> > >> > > > > >> > >> > package, > >> > >> > > > > >> > >> > make authorization interface follow kafka’s > >> coding > >> > >> > > standards > >> > >> > > > > and > >> > >> > > > > >> > will > >> > >> > > > > >> > >> > allow > >> > >> > > > > >> > >> > java implementations to be cleaner. We can > >> either > >> > >> > > completely > >> > >> > > > > drop > >> > >> > > > > >> > >> > scala > >> > >> > > > > >> > >> > interface, which might be a pain for existing > >> > >> > > > > implementations, or > >> > >> > > > > >> > we > >> > >> > > > > >> > >> > can > >> > >> > > > > >> > >> > have scala interface wrap java interface. > Later > >> > >> allows a > >> > >> > > > > cleaner > >> > >> > > > > >> > >> > deprecation path for existing scala authorizer > >> > >> > interface, > >> > >> > > > > however > >> > >> > > > > >> > it > >> > >> > > > > >> > >> > may or > >> > >> > > > > >> > >> > may not be feasible as Kafka server will have > to > >> > >> somehow > >> > >> > > > > decide > >> > >> > > > > >> > which > >> > >> > > > > >> > >> > interface it should be looking for while > loading > >> > >> > > authorizer > >> > >> > > > > >> > >> > implementation, > >> > >> > > > > >> > >> > this can probably be solved with a config or > >> some > >> > >> > > > reflection. > >> > >> > > > > If > >> > >> > > > > >> we > >> > >> > > > > >> > >> > choose > >> > >> > > > > >> > >> > to go this route, I can dig deeper. > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > If we decide to go with option 1, I think it > would > >> be > >> > >> fair > >> > >> > to > >> > >> > > > say > >> > >> > > > > >> that > >> > >> > > > > >> > >> > scala authorizer interface will be around for > some > >> > >> time, as > >> > >> > > > there > >> > >> > > > > >> > will be > >> > >> > > > > >> > >> > more implementations relying on it. If we go with > >> > >> option 2 > >> > >> > > and > >> > >> > > > > >> commit > >> > >> > > > > >> > on > >> > >> > > > > >> > >> > getting this in ASAP, preferably in 0.10, there > >> will > >> > be > >> > >> > fewer > >> > >> > > > > >> > >> > implementations that will be affected. > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > *Another thing to notice is that scala authorizer > >> > >> interface > >> > >> > > is > >> > >> > > > > not > >> > >> > > > > >> > >> > annotated as unstable.* > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > On Wed, Apr 6, 2016 at 9:41 AM, Ashish Singh < > >> > >> > > > > asi...@cloudera.com <javascript:;>> > >> > >> > > > > >> > >> wrote: > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > > I see value in minimizing breaking changes and > I > >> do > >> > >> not > >> > >> > > > oppose > >> > >> > > > > the > >> > >> > > > > >> > >> idea of > >> > >> > > > > >> > >> > > increasing scope of KIP-50 to move auth > >> interface to > >> > >> > java. > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > As authorizer implementations do not really > need > >> to > >> > >> > depend > >> > >> > > on > >> > >> > > > > >> Kafka > >> > >> > > > > >> > >> core, > >> > >> > > > > >> > >> > > I would suggest that we keep authorizer > interface > >> > and > >> > >> its > >> > >> > > > > >> components > >> > >> > > > > >> > >> in a > >> > >> > > > > >> > >> > > separate package. I share the concern that > right > >> now > >> > >> > using > >> > >> > > > > >> Resource, > >> > >> > > > > >> > >> > > Operation, etc, in java implementations is > >> messy. I > >> > >> had > >> > >> > to > >> > >> > > > deal > >> > >> > > > > >> with > >> > >> > > > > >> > >> lot of > >> > >> > > > > >> > >> > > it while writing Apache Sentry plugin. > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > My only ask is to have this in 0.10. As Jay > >> pointed > >> > >> out, > >> > >> > > > right > >> > >> > > > > now > >> > >> > > > > >> > >> there > >> > >> > > > > >> > >> > > are not many implementations out there, we > might > >> > want > >> > >> to > >> > >> > > fix > >> > >> > > > it > >> > >> > > > > >> > ASAP. > >> > >> > > > > >> > >> I can > >> > >> > > > > >> > >> > > only speak of Sentry integration and I think > 0.10 > >> > >> will be > >> > >> > > the > >> > >> > > > > best > >> > >> > > > > >> > for > >> > >> > > > > >> > >> such > >> > >> > > > > >> > >> > > a change, as I should be able to adopt the > >> changes > >> > in > >> > >> > > Sentry > >> > >> > > > > >> > >> integration > >> > >> > > > > >> > >> > > before a lot of users start using it. > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > On Wed, Apr 6, 2016 at 9:25 AM, Ismael Juma < > >> > >> > > > ism...@juma.me.uk > >> > >> > > > > <javascript:;>> > >> > >> > > > > >> > wrote: > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > >> It is small, but breaks binary compatibility. > >> > >> > > > > >> > >> > >> > >> > >> > > > > >> > >> > >> Ismael > >> > >> > > > > >> > >> > >> > >> > >> > > > > >> > >> > >> On Wed, Apr 6, 2016 at 5:20 PM, Grant Henke < > >> > >> > > > > ghe...@cloudera.com <javascript:;> > >> > >> > > > > >> > > >> > >> > > > > >> > >> wrote: > >> > >> > > > > >> > >> > >> > >> > >> > > > > >> > >> > >> > KIP-50 as defined is very small. I don't see > >> any > >> > >> harm > >> > >> > in > >> > >> > > > > >> putting > >> > >> > > > > >> > it > >> > >> > > > > >> > >> in > >> > >> > > > > >> > >> > >> as > >> > >> > > > > >> > >> > >> > is and then tackling the follow up work. > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > On Wed, Apr 6, 2016 at 11:16 AM, Ismael > Juma < > >> > >> > > > > >> ism...@juma.me.uk <javascript:;>> > >> > >> > > > > >> > >> wrote: > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > > Thanks Grant. I wonder if KIP-50 should > >> just be > >> > >> done > >> > >> > > as > >> > >> > > > > part > >> > >> > > > > >> of > >> > >> > > > > >> > >> this > >> > >> > > > > >> > >> > >> > work. > >> > >> > > > > >> > >> > >> > > > >> > >> > > > > >> > >> > >> > > Ismael > >> > >> > > > > >> > >> > >> > > > >> > >> > > > > >> > >> > >> > > On Wed, Apr 6, 2016 at 5:12 PM, Grant > Henke > >> < > >> > >> > > > > >> > ghe...@cloudera.com <javascript:;>> > >> > >> > > > > >> > >> > >> wrote: > >> > >> > > > > >> > >> > >> > > > >> > >> > > > > >> > >> > >> > > > My work with KIP-4 found that many of > the > >> > Scala > >> > >> > > > classes > >> > >> > > > > >> used > >> > >> > > > > >> > in > >> > >> > > > > >> > >> the > >> > >> > > > > >> > >> > >> > > > Authorizer interface are needed in the > >> > Clients > >> > >> > > package > >> > >> > > > > when > >> > >> > > > > >> > >> adding > >> > >> > > > > >> > >> > >> the > >> > >> > > > > >> > >> > >> > > > various ACL requests and responses. I > also > >> > >> found > >> > >> > > that > >> > >> > > > we > >> > >> > > > > >> > don't > >> > >> > > > > >> > >> have > >> > >> > > > > >> > >> > >> > > > standard Exceptions defined for the > >> > authorizer > >> > >> > > > > interface. > >> > >> > > > > >> > This > >> > >> > > > > >> > >> means > >> > >> > > > > >> > >> > >> > that > >> > >> > > > > >> > >> > >> > > > when I add the Authorizer calls to the > >> broker > >> > >> and > >> > >> > > wire > >> > >> > > > > >> > >> protocols all > >> > >> > > > > >> > >> > >> > > > exceptions will be reported as an > "Unknown > >> > >> Error" > >> > >> > > back > >> > >> > > > > to > >> > >> > > > > >> the > >> > >> > > > > >> > >> user > >> > >> > > > > >> > >> > >> via > >> > >> > > > > >> > >> > >> > > the > >> > >> > > > > >> > >> > >> > > > wire protocol. > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > I have written more about it on the > KIP-4 > >> > wiki > >> > >> and > >> > >> > > > > created > >> > >> > > > > >> > >> jiras to > >> > >> > > > > >> > >> > >> > track > >> > >> > > > > >> > >> > >> > > > those issues (See below). I think we > >> should > >> > >> wrap > >> > >> > up > >> > >> > > > this > >> > >> > > > > >> KIP > >> > >> > > > > >> > as > >> > >> > > > > >> > >> is > >> > >> > > > > >> > >> > >> and > >> > >> > > > > >> > >> > >> > > > tackle the Java/Exception changes as a > >> part > >> > of > >> > >> > those > >> > >> > > > > >> > jiras/kips. > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > - KIP-4 "Follow Up Changes" > >> > >> > > > > >> > >> > >> > > > < > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > >> > >> > > > > >> > >> > >> > >> > > > > >> > > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-FollowUpChangesfollow-up-changes > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > - KAFKA-3509 < > >> > >> > > > > >> > >> https://issues.apache.org/jira/browse/KAFKA-3509>: > >> > >> > > > > >> > >> > >> > > > Provide > >> > >> > > > > >> > >> > >> > > > an Authorizer interface using the > Java > >> > >> client > >> > >> > > > > enumerator > >> > >> > > > > >> > >> classes > >> > >> > > > > >> > >> > >> > > > - KAFKA-3507 < > >> > >> > > > > >> > >> https://issues.apache.org/jira/browse/KAFKA-3507>: > >> > >> > > > > >> > >> > >> > > Define > >> > >> > > > > >> > >> > >> > > > standard exceptions for the > Authorizer > >> > >> > interface > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > Thank you, > >> > >> > > > > >> > >> > >> > > > Grant > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > On Wed, Apr 6, 2016 at 10:58 AM, Jay > >> Kreps < > >> > >> > > > > >> j...@confluent.io <javascript:;> > >> > >> > > > > >> > > > >> > >> > > > > >> > >> > >> wrote: > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > > Hey Ismael, > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > > Yeah I think this is a minor > cleanliness > >> > >> thing. > >> > >> > > > Since > >> > >> > > > > >> this > >> > >> > > > > >> > is > >> > >> > > > > >> > >> kind > >> > >> > > > > >> > >> > >> > of a > >> > >> > > > > >> > >> > >> > > > > power user interface I don't feel > >> strongly > >> > >> > either > >> > >> > > > way. > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > > My motivation with Scala is just that > >> we've > >> > >> > tried > >> > >> > > to > >> > >> > > > > move > >> > >> > > > > >> > to > >> > >> > > > > >> > >> > >> having > >> > >> > > > > >> > >> > >> > the > >> > >> > > > > >> > >> > >> > > > > public interfaces be Java, and as a > >> group > >> > we > >> > >> > > > > definitely > >> > >> > > > > >> > >> struggled > >> > >> > > > > >> > >> > >> a > >> > >> > > > > >> > >> > >> > lot > >> > >> > > > > >> > >> > >> > > > > with understanding and maintaining > Scala > >> > >> > > > > compatibility in > >> > >> > > > > >> > the > >> > >> > > > > >> > >> > >> older > >> > >> > > > > >> > >> > >> > > > > clients. > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > > -Jay > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > > On Tue, Apr 5, 2016 at 11:46 PM, > Ismael > >> > Juma > >> > >> < > >> > >> > > > > >> > >> ism...@juma.me.uk <javascript:;>> > >> > >> > > > > >> > >> > >> > > wrote: > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > > > Hi Jay, > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > On Wed, Apr 6, 2016 at 3:48 AM, Jay > >> > Kreps < > >> > >> > > > > >> > j...@confluent.io <javascript:;> > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > >> > wrote: > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > > Given that we're breaking > >> compatibility > >> > >> > anyway > >> > >> > > > > should > >> > >> > > > > >> > we: > >> > >> > > > > >> > >> > >> > > > > > > > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > We are not breaking source > >> compatibility > >> > >> since > >> > >> > > the > >> > >> > > > > new > >> > >> > > > > >> > >> method > >> > >> > > > > >> > >> > >> has a > >> > >> > > > > >> > >> > >> > > > > default > >> > >> > > > > >> > >> > >> > > > > > implementation. I take it that you > >> mean > >> > >> binary > >> > >> > > > > >> > >> compatibility? > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > > 1. Remove the get prefix on this > >> method > >> > >> and > >> > >> > > the > >> > >> > > > > >> > existing > >> > >> > > > > >> > >> one > >> > >> > > > > >> > >> > >> > which > >> > >> > > > > >> > >> > >> > > > > > violate > >> > >> > > > > >> > >> > >> > > > > > > our own code style guidelines > (Oops! > >> > >> Kind of > >> > >> > > sad > >> > >> > > > > we > >> > >> > > > > >> > went > >> > >> > > > > >> > >> > >> through > >> > >> > > > > >> > >> > >> > > the > >> > >> > > > > >> > >> > >> > > > > > whole > >> > >> > > > > >> > >> > >> > > > > > > KIP process and no one even > flagged > >> > this) > >> > >> > > > > >> > >> > >> > > > > > > > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > I did flag this during the > discussion > >> and > >> > >> > Ashish > >> > >> > > > > said > >> > >> > > > > >> he > >> > >> > > > > >> > >> would > >> > >> > > > > >> > >> > >> > change > >> > >> > > > > >> > >> > >> > > > it > >> > >> > > > > >> > >> > >> > > > > if > >> > >> > > > > >> > >> > >> > > > > > other people felt that it should be > >> > >> changed. > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > > 2. Move the interface out of scala > >> to > >> > be > >> > >> a > >> > >> > > > normal > >> > >> > > > > >> Java > >> > >> > > > > >> > >> > >> interface > >> > >> > > > > >> > >> > >> > > > > > > > >> > >> > > > > >> > >> > >> > > > > > > This breaks source compatibility > but > >> > >> > probably > >> > >> > > > > what we > >> > >> > > > > >> > >> should > >> > >> > > > > >> > >> > >> have > >> > >> > > > > >> > >> > >> > > > done > >> > >> > > > > >> > >> > >> > > > > > > originally I suspect. Probably > there > >> > are > >> > >> few > >> > >> > > > > enough > >> > >> > > > > >> > >> > >> > implementations > >> > >> > > > > >> > >> > >> > > > of > >> > >> > > > > >> > >> > >> > > > > > this > >> > >> > > > > >> > >> > >> > > > > > > that it is better to just rip the > >> > bandaid > >> > >> > off. > >> > >> > > > > >> > >> > >> > > > > > > > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > Can you please explain the > >> motivation? It > >> > >> did > >> > >> > > come > >> > >> > > > > up > >> > >> > > > > >> in > >> > >> > > > > >> > >> > >> previous > >> > >> > > > > >> > >> > >> > > > > > discussions that some things like > >> > Operation > >> > >> > and > >> > >> > > > > >> > ResourceType > >> > >> > > > > >> > >> > >> should > >> > >> > > > > >> > >> > >> > > be > >> > >> > > > > >> > >> > >> > > > in > >> > >> > > > > >> > >> > >> > > > > > the clients library, but not > >> Authorizer > >> > >> > itself. > >> > >> > > > Are > >> > >> > > > > we > >> > >> > > > > >> > >> saying > >> > >> > > > > >> > >> > >> that > >> > >> > > > > >> > >> > >> > > any > >> > >> > > > > >> > >> > >> > > > > > pluggable interface should be in > Java > >> so > >> > >> that > >> > >> > > > users > >> > >> > > > > can > >> > >> > > > > >> > >> > >> implement > >> > >> > > > > >> > >> > >> > it > >> > >> > > > > >> > >> > >> > > > > > without including the Scala library? > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > Grant, you originally suggested that > >> some > >> > >> > things > >> > >> > > > > would > >> > >> > > > > >> > have > >> > >> > > > > >> > >> to > >> > >> > > > > >> > >> > >> be > >> > >> > > > > >> > >> > >> > in > >> > >> > > > > >> > >> > >> > > > the > >> > >> > > > > >> > >> > >> > > > > > Java side as well. Can you please > >> > >> elaborate on > >> > >> > > > this? > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > Ismael > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > >> > >> > >> > > > > > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > -- > >> > >> > > > > >> > >> > >> > > > Grant Henke > >> > >> > > > > >> > >> > >> > > > Software Engineer | Cloudera > >> > >> > > > > >> > >> > >> > > > gr...@cloudera.com <javascript:;> | > >> > >> > > > twitter.com/gchenke > >> > >> > > > > | > >> > >> > > > > >> > >> > >> linkedin.com/in/granthenke > >> > >> > > > > >> > >> > >> > > > > >> > >> > > > > >> > >> > >> > > > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > -- > >> > >> > > > > >> > >> > >> > Grant Henke > >> > >> > > > > >> > >> > >> > Software Engineer | Cloudera > >> > >> > > > > >> > >> > >> > gr...@cloudera.com <javascript:;> | > >> > >> > twitter.com/gchenke > >> > >> > > | > >> > >> > > > > >> > >> linkedin.com/in/granthenke > >> > >> > > > > >> > >> > >> > > >> > >> > > > > >> > >> > >> > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > -- > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > Regards, > >> > >> > > > > >> > >> > > Ashish > >> > >> > > > > >> > >> > > > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > -- > >> > >> > > > > >> > >> > > >> > >> > > > > >> > >> > Regards, > >> > >> > > > > >> > >> > Ashish > >> > >> > > > > >> > >> > >> > >> > > > > >> > > > >> > >> > > > > >> > >-- > >> > >> > > > > >> > > > >> > >> > > > > >> > >Regards, > >> > >> > > > > >> > >Ashish > >> > >> > > > > >> > > >> > >> > > > > >> > > >> > >> > > > > >> > >> > >> > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> > >> > > > > >-- > >> > >> > > > > > > >> > >> > > > > >Regards, > >> > >> > > > > >Ashish > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > -- > >> > >> > > > Ashish 🎤h > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > > >> > >> > > >> > >> > -- > >> > >> > > >> > >> > Regards, > >> > >> > Ashish > >> > >> > > >> > >> > >> > > > >> > > > >> > > > >> > > -- > >> > > > >> > > Regards, > >> > > Ashish > >> > > > >> > > >> > > >> > > >> > -- > >> > > >> > Regards, > >> > Ashish > >> > > >> > >> > >> > >> -- > >> Grant Henke > >> Software Engineer | Cloudera > >> gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke > >> > > > > > > > > -- > > > > Regards, > > Ashish > > > > > > -- > > Regards, > Ashish >
