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