@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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>
>
signature.asc
Description: OpenPGP digital signature
