kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andy Coates <a...@confluent.io>
Subject Re: [VOTE] KIP-476: Add Java AdminClient interface
Date Wed, 03 Jul 2019 12:03:24 GMT
Matthias,

I was referring to platforms such as spark or flink that support multiple
versions of the Kafka clients. Ismael mentioned this higher up on the
thread.

I'd prefer this KIP didn't get held up over somewhat unrelated change, i.e.
should the factory method be on the interface or utility class.  Surely,
now would be a great time to change this if we wanted, but we can also
change this later if we need to.  In the interest of moving forward, can I
propose we leave the factory methods as they are in the KIP?

Thanks,

Andy

On Tue, 2 Jul 2019 at 17:14, Colin McCabe <cmccabe@apache.org> wrote:

> 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 <matthias@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.duggana@gmail.com>
> > > >>> wrote:
> > > >>>
> > > >>>> +1 (non-binding)
> > > >>>>
> > > >>>> On Wed, Jun 26, 2019 at 8:37 AM Satish Duggana <
> > > >> satish.duggana@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 <andy@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 <
> matthias@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
<
> andy@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
<ismael@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 <
> andy@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 <
> > > >>>> ryannedolan@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 <
> > > >>>> andy@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.duggana@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 <
> > > >>>> andy@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 <
> > > >>>>>>>>>>> matthias@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 <
> > > >>>>>>>>>>> ryannedolan@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 <
> > > >>>> andy@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
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message