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