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> wrote: >Hello Harsha, > >On Thu, Apr 7, 2016 at 11:03 AM, Harsha <m...@harsha.io> 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> >> 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> wrote: >> > > >> > >> It is small, but breaks binary compatibility. >> > >> >> > >> Ismael >> > >> >> > >> On Wed, Apr 6, 2016 at 5:20 PM, Grant Henke <ghe...@cloudera.com> >> 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> >> 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> >> > >> 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> >> > >> 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> >> > >> > > wrote: >> > >> > > > > >> > >> > > > > > Hi Jay, >> > >> > > > > > >> > >> > > > > > On Wed, Apr 6, 2016 at 3:48 AM, Jay Kreps <j...@confluent.io >> > >> > >> > 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 | twitter.com/gchenke | >> > >> linkedin.com/in/granthenke >> > >> > > > >> > >> > > >> > >> > >> > >> > >> > >> > >> > >> > -- >> > >> > Grant Henke >> > >> > Software Engineer | Cloudera >> > >> > gr...@cloudera.com | twitter.com/gchenke | >> linkedin.com/in/granthenke >> > >> > >> > >> >> > > >> > > >> > > >> > > -- >> > > >> > > Regards, >> > > Ashish >> > > >> > >> > >> > >> > -- >> > >> > Regards, >> > Ashish >> > >-- > >Regards, >Ashish
