From dev-return-105428-archive-asf-public=cust-asf.ponee.io@kafka.apache.org Mon Jul 1 11:22:47 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id CAD95180670 for ; Mon, 1 Jul 2019 13:22:46 +0200 (CEST) Received: (qmail 13202 invoked by uid 500); 1 Jul 2019 11:22:44 -0000 Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kafka.apache.org Delivered-To: mailing list dev@kafka.apache.org Received: (qmail 13186 invoked by uid 99); 1 Jul 2019 11:22:44 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 01 Jul 2019 11:22:44 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 75E2A1810C5 for ; Mon, 1 Jul 2019 11:22:43 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.683 X-Spam-Level: * X-Spam-Status: No, score=1.683 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.118, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd3-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=confluent.io Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id lE9wgkfNwY99 for ; Mon, 1 Jul 2019 11:22:41 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=209.85.208.65; helo=mail-ed1-f65.google.com; envelope-from=andy@confluent.io; receiver= Received: from mail-ed1-f65.google.com (mail-ed1-f65.google.com [209.85.208.65]) by mx1-ec2-va.apache.org (ASF Mail Server at mx1-ec2-va.apache.org) with ESMTPS id 743A6BC76E for ; Mon, 1 Jul 2019 11:22:40 +0000 (UTC) Received: by mail-ed1-f65.google.com with SMTP id m10so22875397edv.6 for ; Mon, 01 Jul 2019 04:22:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=confluent.io; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=DDvScYCRiKeGZ+JUPsBzM1eDYqoWJLj7DO4kPr8Aa5c=; b=Hs/hiCXnFtOydyZxeD6ko6oZWV60GW28QrBvmwKnAQYcA618HBgBeWvRJZWXSKxaMR 9wIlEh/CF48TY0CBZHz1Nmgk7dK1a7JTSACgS6j43ZAWaEnCJ72PCyrUT95EiZO8P1d+ 6/y6knOtIW7uRXT6In/cbVkD73Fc5OGg+1lfy3BwaYqaA42y+OkSoSO9y6zBCtW3Y0Yu ZttPYFDIfZQNxSkaUqiv6HxFLK7ibrLzd9lvHX5aqivF6nUBs/XUM1Wcd0WPiLP4ZvgS mQVWMDRca+K8NhrshkkfVKbw/w/lfgiCMcdL4Haccj0Ytv6rXtVbrIGFvUum1igcJT0Y pnRA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=DDvScYCRiKeGZ+JUPsBzM1eDYqoWJLj7DO4kPr8Aa5c=; b=OdXrPzRGLvF/6/4kXFOFwp9Hd27/OxtdSQtrJFIJ+MYZgP9RwbKhEjhncLHTZRrzwd 9/nVmQ8U11oD9KVikEH7Ewih/iDRf5na6gb8KIpcA80hNWldGmepTqI0iX2pck37NHak U8YBmUoGlIGu74CXAW9FQ5hiDOE96eoSq8fwdpUBJ2dTrVy8seWEYXkqD3Wl1oJHC35q wYjWoFdpu+Fu/fQNbYdl14Twhr7gk94pYLIWV0kMUCXBMWi4+kstK0GuHI8x/fVji8+l 5b1f84xPAK5sH2WrXeM5InS3cH5+S7llBpB8nwugdbTtCPeWa0kYPu6il/AGC83AN5Ny cV8g== X-Gm-Message-State: APjAAAX3BkrfZRPNPoPu1URmoASPY8Ii2bztLcYvT/xkNJMJn1u5cYOd zxqPCDwFnITOtZz6bMFlMraMi8S9ccmeWGeyYfqe7f7v X-Google-Smtp-Source: APXvYqzuen3HoKA+7utpHbxDDbpRDkZf7rO7M+CmT72sKF7lUkHkFdhQoftze3lLW0O2z/RyEcl7+xykmexRSwVwFoo= X-Received: by 2002:a50:91ae:: with SMTP id g43mr28355986eda.279.1561980159018; Mon, 01 Jul 2019 04:22:39 -0700 (PDT) MIME-Version: 1.0 References: <334f5d1a-049b-ea26-bfdb-274c71ab20a0@confluent.io> <7AD3B8C7-04E4-49E2-A036-8D8FE918607F@confluent.io> <8923d8f3-61fd-e213-517a-de701a6dd7b5@confluent.io> In-Reply-To: From: Andy Coates Date: Mon, 1 Jul 2019 12:22:27 +0100 Message-ID: Subject: Re: [VOTE] KIP-476: Add Java AdminClient interface To: dev Content-Type: multipart/alternative; boundary="0000000000008664db058c9cd85e" --0000000000008664db058c9cd85e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 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 > >> wrote: > >>>> > >>>>> I still think, that an interface does not need to know anything abo= ut > >>>>> its implementation. But I am also fine if we add a factory method t= o > >> 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 > >> wrote: > >>>>>> > >>>>>>> Hi Ismael, > >>>>>>> > >>>>>>> I=E2=80=99m happy enough to not deprecate the existing `AdminClie= nt` > >> 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=E2=80= =99t 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=E2=80=99s my thinking for deprecation, but as I=E2=80=99ve s= aid I=E2=80=99m happy > >> either > >>>>> way. > >>>>>>> > >>>>>>> Andy > >>>>>>> > >>>>>>>> On 18 Jun 2019, at 16:09, Ismael Juma 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 > >> 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 othe= r > >>>>>>>>>> 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+Adm= inClient+Interface > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Andy > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>>> > >> > > > > --0000000000008664db058c9cd85e--