Hi All,
So voting currently stands on:
Binding:
+1 Matthias,
+1 Colin
Non-binding:
+1 Thomas Becker
+1 Satish Guggana
+1 Ryan Dolan
So we're still 1 binding vote short. :(
On Wed, 3 Jul 2019 at 23:08, Matthias J. Sax <matth...@confluent.io> wrote:
> Thanks for the details Colin and Andy.
>
> My indent was not to block the KIP, but it seems to be a fair question
> to ask.
>
> I talked to Ismael offline about it and understand his reasoning better
> now. If we don't deprecate `abstract AdminClient` class, it seems
> reasonable to not deprecate the corresponding factory methods either.
>
>
> +1 (binding) on the current proposal
>
>
>
> -Matthias
>
> On 7/3/19 5:03 AM, Andy Coates wrote:
> > 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 <cmcc...@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 <matth...@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.dugg...@gmail.com>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> +1 (non-binding)
> >>>>>>>>
> >>>>>>>> On Wed, Jun 26, 2019 at 8:37 AM Satish Duggana <
> >>>>>> satish.dugg...@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 <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
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>> Attachments:
> >>>> * signature.asc
> >>>
> >>
> >
>
>
