> > The follow-up question is where these classes should live. We have the > common package in clients for common code. However, if something is > exclusively used by the broker, we could add it under core/src/main/java > instead. There are pros and cons for each approach, so I was wondering if > some thought has gone into this already.
Just adding my 2 cents here. Since the implemented Authorizer needs to exist on the brokers classpath, I think the interface should be a part of the core package regardless of any Java vs Scala choice. Its components, that need to be used in the clients for requests/responses should be in the clients/common package though. Some of this has been done in my preliminary ACLs request/response pull request: https://github.com/apache/kafka/pull/1005 On Wed, Apr 6, 2016 at 11:20 AM, 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 > -- Grant Henke Software Engineer | Cloudera gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
