+1 (binding) Thank you for the improvement.
On Thu, Jul 11, 2019, 3:53 AM Andy Coates <a...@confluent.io> wrote: > Hi All, > > So voting currently stands on: > > Binding: > +1 Matthias, > +1 Colin > > Non-binding: > +1 Thomas Becker > +1 Satish Guggana > +1 Ryan Dolan > > So we're still 1 binding vote short. :( > > > On Wed, 3 Jul 2019 at 23:08, Matthias J. Sax <matth...@confluent.io> > wrote: > > > Thanks for the details Colin and Andy. > > > > My indent was not to block the KIP, but it seems to be a fair question > > to ask. > > > > I talked to Ismael offline about it and understand his reasoning better > > now. If we don't deprecate `abstract AdminClient` class, it seems > > reasonable to not deprecate the corresponding factory methods either. > > > > > > +1 (binding) on the current proposal > > > > > > > > -Matthias > > > > On 7/3/19 5:03 AM, Andy Coates wrote: > > > Matthias, > > > > > > I was referring to platforms such as spark or flink that support > multiple > > > versions of the Kafka clients. Ismael mentioned this higher up on the > > > thread. > > > > > > I'd prefer this KIP didn't get held up over somewhat unrelated change, > > i.e. > > > should the factory method be on the interface or utility class. > Surely, > > > now would be a great time to change this if we wanted, but we can also > > > change this later if we need to. In the interest of moving forward, > can > > I > > > propose we leave the factory methods as they are in the KIP? > > > > > > Thanks, > > > > > > Andy > > > > > > On Tue, 2 Jul 2019 at 17:14, Colin McCabe <cmcc...@apache.org> wrote: > > > > > >> On Tue, Jul 2, 2019, at 09:14, Colin McCabe wrote: > > >>> On Mon, Jul 1, 2019, at 23:30, Matthias J. Sax wrote: > > >>>> Not sure, if I understand the argument? > > >>>> > > >>>> Why would anyone need to support multiple client side versions? > > >>>> Clients/brokers are forward/backward compatible anyway. > > >>> > > >>> When you're using many different libraries, it is helpful if they > don't > > >>> impose tight constraints on what versions their dependencies are. > > >>> Otherwise you can easily get in a situation where the constraints > can't > > >>> be satisfied. > > >>> > > >>>> > > >>>> Also, if one really supports multiple client side versions, won't > they > > >>>> use multiple shaded dependencies for different versions? > > >>> > > >>> Shading the Kafka client doesn't really work, because of how we use > > >> reflection. > > >>> > > >>>> > > >>>> Last, it's possible to suppress warnings (at least in Java). > > >>> > > >>> But not in Scala. So that does not help (for example), Scala users. > > >> > > >> I meant to write "Spark users" here. > > >> > > >> C. > > >> > > >>> > > >>> I agree that in general we should be using deprecation when > > >>> appropriate, regardless of the potential annoyances to users. But > I'm > > >>> not sure deprecating this method is really worth it. > > >>> > > >>> best, > > >>> Colin > > >>> > > >>> > > >>>> > > >>>> Can you elaborate? > > >>>> > > >>>> IMHO, just adding a statement to JavaDocs is a little weak, and at > > some > > >>>> point, we need to deprecate those methods anyway if we ever want to > > >>>> remove them. The earlier we deprecate them, the earlier we can > remove > > >> them. > > >>>> > > >>>> > > >>>> -Matthias > > >>>> > > >>>> On 7/1/19 4:22 AM, Andy Coates wrote: > > >>>>> Done. I've not deprecated the factory methods on the AdminClient > for > > >> the > > >>>>> same reason the AdminClient itself is not deprecated, i.e. this > > >> would cause > > >>>>> unavoidable warnings for libraries / platforms that support > multiple > > >>>>> versions of Kafka. However, I think we add a note to the Java docs > of > > >>>>> `AdminClient` to indicate that its use, going forward, is > > >> discouraged in > > >>>>> favour of the new `Admin` interface and explain why its not been > > >>>>> deprecated, but that it may/will be removed in a future version. > > >>>>> > > >>>>> Regarding factory methods on interfaces: there seems to be some > > >> difference > > >>>>> of opinion here. I'm not sure of the best approach to revolve this. > > >> At the > > >>>>> moment the KIP has factory methods on the new `Admin` interface, > > >> rather > > >>>>> than some utility class. I prefer the utility class, but this isn't > > >> inline > > >>>>> with the patterns in the code base and some of the core team have > > >> expressed > > >>>>> they'd prefer to continue to have the factory methods on the > > >> interface. > > >>>>> I'm happy with this if others are. > > >>>>> > > >>>>> Thanks, > > >>>>> > > >>>>> Andy > > >>>>> > > >>>>> On Thu, 27 Jun 2019 at 23:21, Matthias J. Sax < > matth...@confluent.io > > > > > >> wrote: > > >>>>> > > >>>>>> @Andy: > > >>>>>> > > >>>>>> What about the factory methods of `AdminClient` class? Should they > > >> be > > >>>>>> deprecated? > > >>>>>> > > >>>>>> One nit about the KIP: can you maybe insert "code blocks" to > > >> highlight > > >>>>>> the API changes? Code blocks would simplify to read the KIP a lot. > > >>>>>> > > >>>>>> > > >>>>>> -Matthias > > >>>>>> > > >>>>>> On 6/26/19 6:56 AM, Ryanne Dolan wrote: > > >>>>>>> +1 (non-binding) > > >>>>>>> > > >>>>>>> Thanks. > > >>>>>>> Ryanne > > >>>>>>> > > >>>>>>> On Tue, Jun 25, 2019 at 10:21 PM Satish Duggana < > > >>>>>> satish.dugg...@gmail.com> > > >>>>>>> wrote: > > >>>>>>> > > >>>>>>>> +1 (non-binding) > > >>>>>>>> > > >>>>>>>> On Wed, Jun 26, 2019 at 8:37 AM Satish Duggana < > > >>>>>> satish.dugg...@gmail.com> > > >>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>> +1 Matthias/Andy. > > >>>>>>>>> IMHO, interface is about the contract, it should not > have/expose > > >> any > > >>>>>>>>> implementation. I am fine with either way as it is more of > taste > > >> or > > >>>>>>>>> preference. > > >>>>>>>>> > > >>>>>>>>> Agree with Ismael/Colin/Ryanne on not deprecating for good > > >> reasons. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> On Mon, Jun 24, 2019 at 8:33 PM Andy Coates <a...@confluent.io > > > > >> wrote: > > >>>>>>>>>> > > >>>>>>>>>> I agree Matthias. > > >>>>>>>>>> > > >>>>>>>>>> (In Scala, such factory methods are on a companion object. As > > >> Java > > >>>>>>>> doesn't > > >>>>>>>>>> have the concept of a companion object, an equivalent would > be a > > >>>>>>>> utility > > >>>>>>>>>> class with a similar name...) > > >>>>>>>>>> > > >>>>>>>>>> However, I'll update the KIP to include the factory method on > > >> the > > >>>>>>>> interface. > > >>>>>>>>>> > > >>>>>>>>>> On Fri, 21 Jun 2019 at 23:40, Matthias J. Sax < > > >> matth...@confluent.io> > > >>>>>>>> wrote: > > >>>>>>>>>> > > >>>>>>>>>>> I still think, that an interface does not need to know > > >> anything about > > >>>>>>>>>>> its implementation. But I am also fine if we add a factory > > >> method to > > >>>>>>>> the > > >>>>>>>>>>> new interface if that is preferred by most people. > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> -Matthias > > >>>>>>>>>>> > > >>>>>>>>>>> On 6/21/19 7:10 AM, Ismael Juma wrote: > > >>>>>>>>>>>> This is even more reason not to deprecate immediately, there > > >> is > > >>>>>>>> very > > >>>>>>>>>>> little > > >>>>>>>>>>>> maintenance cost for us. We should be mindful that many of > our > > >>>>>>>> users (eg > > >>>>>>>>>>>> Spark, Flink, etc.) typically allow users to specify the > kafka > > >>>>>>>> clients > > >>>>>>>>>>>> version and hence avoid using new classes/interfaces for > some > > >>>>>>>> time. They > > >>>>>>>>>>>> would get a bunch of warnings they cannot do anything about > > >> apart > > >>>>>>>> from > > >>>>>>>>>>>> suppressing. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Ismael > > >>>>>>>>>>>> > > >>>>>>>>>>>> On Fri, Jun 21, 2019 at 4:00 AM Andy Coates < > > >> a...@confluent.io> > > >>>>>>>> wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>>> Hi Ismael, > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> I’m happy enough to not deprecate the existing > `AdminClient` > > >>>>>>>> class as > > >>>>>>>>>>> part > > >>>>>>>>>>>>> of this change. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> However, note that, the class will likely be empty, i.e. > all > > >>>>>>>> methods and > > >>>>>>>>>>>>> implementations will be inherited from the interface: > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> public abstract class AdminClient implements Admin { > > >>>>>>>>>>>>> } > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Not marking it as deprecated has the benefit that users > > >> won’t see > > >>>>>>>> any > > >>>>>>>>>>>>> deprecation warnings on the next release. Conversely, > > >> deprecating > > >>>>>>>> it > > >>>>>>>>>>> will > > >>>>>>>>>>>>> mean we can choose to remove this, now pointless class, in > > >> the > > >>>>>>>> future > > >>>>>>>>>>> if we > > >>>>>>>>>>>>> choose. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> That’s my thinking for deprecation, but as I’ve said I’m > > >> happy > > >>>>>>>> either > > >>>>>>>>>>> way. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Andy > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> On 18 Jun 2019, at 16:09, Ismael Juma <ism...@juma.me.uk> > > >> wrote: > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> I agree with Ryanne, I think we should avoid deprecating > > >>>>>>>> AdminClient > > >>>>>>>>>>> and > > >>>>>>>>>>>>>> causing so much churn for users who don't actually care > > >> about > > >>>>>>>> this > > >>>>>>>>>>> niche > > >>>>>>>>>>>>>> use case. > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> Ismael > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> On Tue, Jun 18, 2019 at 6:43 AM Andy Coates < > > >> a...@confluent.io> > > >>>>>>>> wrote: > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Hi Ryanne, > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> If we don't change the client code, then everywhere will > > >> still > > >>>>>>>> expect > > >>>>>>>>>>>>>>> subclasses of `AdminClient`, so the interface will be of > no > > >>>>>>>> use, i.e. > > >>>>>>>>>>> I > > >>>>>>>>>>>>>>> can't write a class that implements the new interface and > > >> pass > > >>>>>>>> it to > > >>>>>>>>>>> the > > >>>>>>>>>>>>>>> client code. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Andy > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> On Mon, 17 Jun 2019 at 19:01, Ryanne Dolan < > > >>>>>>>> ryannedo...@gmail.com> > > >>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Andy, while I agree that the new interface is useful, > I'm > > >> not > > >>>>>>>>>>> convinced > > >>>>>>>>>>>>>>>> adding an interface requires deprecating AdminClient and > > >>>>>>>> changing so > > >>>>>>>>>>>>> much > > >>>>>>>>>>>>>>>> client code. Why not just add the Admin interface, have > > >>>>>>>> AdminClient > > >>>>>>>>>>>>>>>> implement it, and have done? > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Ryanne > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> On Mon, Jun 17, 2019 at 12:09 PM Andy Coates < > > >>>>>>>> a...@confluent.io> > > >>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> Hi all, > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> I think I've addressed all concerns. Let me know if > I've > > >>>>>>>> not. Can I > > >>>>>>>>>>>>>>> call > > >>>>>>>>>>>>>>>>> another round of votes please? > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> Andy > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> On Fri, 14 Jun 2019 at 04:55, Satish Duggana < > > >>>>>>>>>>>>> satish.dugg...@gmail.com > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> Hi Andy, > > >>>>>>>>>>>>>>>>>> Thanks for the KIP. This is a good change and it gives > > >> the > > >>>>>>>> user a > > >>>>>>>>>>>>>>>> better > > >>>>>>>>>>>>>>>>>> handle on Admin client usage. I agree with the > proposal > > >>>>>>>> except the > > >>>>>>>>>>>>>>> new > > >>>>>>>>>>>>>>>>>> `Admin` interface having all the methods from > > >> `AdminClient` > > >>>>>>>>>>> abstract > > >>>>>>>>>>>>>>>>> class. > > >>>>>>>>>>>>>>>>>> It should be kept clean having only the admin > > >> operations as > > >>>>>>>> methods > > >>>>>>>>>>>>>>>> from > > >>>>>>>>>>>>>>>>>> KafkaClient abstract class but not the factory methods > > >> as > > >>>>>>>> mentioned > > >>>>>>>>>>>>>>> in > > >>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>> earlier mail. > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> I know about dynamic proxies(which were widely used in > > >>>>>>>> RMI/EJB > > >>>>>>>>>>>>>>> world). > > >>>>>>>>>>>>>>>> I > > >>>>>>>>>>>>>>>>> am > > >>>>>>>>>>>>>>>>>> curious about the usecase using dynamic proxies with > > >> Admin > > >>>>>>>> client > > >>>>>>>>>>>>>>>>>> interface. Dynamic proxy can have performance penalty > > >> if it > > >>>>>>>> is used > > >>>>>>>>>>>>>>> in > > >>>>>>>>>>>>>>>>>> critical path. Is that the primary motivation for > > >> creating > > >>>>>>>> the KIP? > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>> Satish. > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> On Wed, Jun 12, 2019 at 8:43 PM Andy Coates < > > >>>>>>>> a...@confluent.io> > > >>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> I'm not married to that part. That was only done to > > >> keep > > >>>>>>>> it more > > >>>>>>>>>>>>>>> or > > >>>>>>>>>>>>>>>>> less > > >>>>>>>>>>>>>>>>>>> inline with what's already there, (an abstract class > > >> that > > >>>>>>>> has a > > >>>>>>>>>>>>>>>> factory > > >>>>>>>>>>>>>>>>>>> method that returns a subclass.... sounds like the > same > > >>>>>>>>>>>>>>> anti-pattern > > >>>>>>>>>>>>>>>>> ;)) > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> An alternative would to have an `AdminClients` > utility > > >>>>>>>> class to > > >>>>>>>>>>>>>>>> create > > >>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>> admin client. > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> On Mon, 10 Jun 2019 at 19:31, Matthias J. Sax < > > >>>>>>>>>>>>>>> matth...@confluent.io > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> Hmmm... > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> So the new interface, returns an instance of a class > > >> that > > >>>>>>>>>>>>>>>> implements > > >>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>> interface. This sounds a little bit like an > > >> anti-pattern? > > >>>>>>>>>>>>>>> Shouldn't > > >>>>>>>>>>>>>>>>>>>> interfaces actually not know anything about classes > > >> that > > >>>>>>>>>>>>>>> implement > > >>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>> interface? > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> -Matthias > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> On 6/10/19 11:22 AM, Andy Coates wrote: > > >>>>>>>>>>>>>>>>>>>>> `AdminClient` would be deprecated purely because it > > >> would > > >>>>>>>> no > > >>>>>>>>>>>>>>>> longer > > >>>>>>>>>>>>>>>>>>> serve > > >>>>>>>>>>>>>>>>>>>>> any purpose and would be virtually empty, getting > > >> all of > > >>>>>>>> its > > >>>>>>>>>>>>>>>>>>>> implementation > > >>>>>>>>>>>>>>>>>>>>> from the new interfar. It would be nice to remove > > >> this > > >>>>>>>> from the > > >>>>>>>>>>>>>>>> API > > >>>>>>>>>>>>>>>>>> at > > >>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>> next major version bump, hence the need to > deprecate. > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> `AdminClient.create()` would return what it does > > >> today, > > >>>>>>>> (so > > >>>>>>>>>>>>>>> not a > > >>>>>>>>>>>>>>>>>>>> breaking > > >>>>>>>>>>>>>>>>>>>>> change). > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> On Tue, 4 Jun 2019 at 22:24, Ryanne Dolan < > > >>>>>>>>>>>>>>> ryannedo...@gmail.com > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> The existing `AdminClient` will be marked as > > >> deprecated. > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> What's the reasoning behind this? I'm fine with > the > > >> other > > >>>>>>>>>>>>>>>> changes, > > >>>>>>>>>>>>>>>>>> but > > >>>>>>>>>>>>>>>>>>>>>> would prefer to keep the existing public API > intact > > >> if > > >>>>>>>> it's > > >>>>>>>>>>>>>>> not > > >>>>>>>>>>>>>>>>>>> hurting > > >>>>>>>>>>>>>>>>>>>>>> anything. > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> Also, what will AdminClient.create() return? Would > > >> it be > > >>>>>>>> a > > >>>>>>>>>>>>>>>>> breaking > > >>>>>>>>>>>>>>>>>>>> change? > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> Ryanne > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> On Tue, Jun 4, 2019, 11:17 AM Andy Coates < > > >>>>>>>> a...@confluent.io> > > >>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Hi folks > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> As there's been no chatter on this KIP I'm > > >> assuming it's > > >>>>>>>>>>>>>>>>>>>> non-contentious, > > >>>>>>>>>>>>>>>>>>>>>>> (or just boring), hence I'd like to call a vote > for > > >>>>>>>> KIP-476: > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>> > > >>>>>> > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-476%3A+Add+Java+AdminClient+Interface > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Andy > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>>> > > >>>> Attachments: > > >>>> * signature.asc > > >>> > > >> > > > > > > > >