Hi all,
KIP updated:
- No deprecation
- Factory method back onto Admin interface
I'd like to kick off another round of voting please.
Thanks,
Andy
On Mon, 24 Jun 2019 at 16:03, 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
>> >>>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>
>> >>>>>
>> >>>>
>> >>
>> >>
>> >
>>
>>
