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