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
