+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
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>
> > > >>
> > > >
> > >
> > >
