kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin McCabe" <cmcc...@apache.org>
Subject Re: [VOTE] KIP-476: Add Java AdminClient interface
Date Tue, 02 Jul 2019 16:13:30 GMT
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 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
View raw message