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:14:03 GMT
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
View raw message