Done.
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
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>
> >
>
>
