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
