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
>
