From dev-return-105427-archive-asf-public=cust-asf.ponee.io@kafka.apache.org Mon Jul 1 11:17:13 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 503C7180670 for ; Mon, 1 Jul 2019 13:17:13 +0200 (CEST) Received: (qmail 95935 invoked by uid 500); 1 Jul 2019 11:17:10 -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 95923 invoked by uid 99); 1 Jul 2019 11:17:09 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 01 Jul 2019 11:17:09 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 3614AC025A for ; Mon, 1 Jul 2019 11:17:09 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.801 X-Spam-Level: * X-Spam-Status: No, score=1.801 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, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd1-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=confluent.io Received: from mx1-he-de.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id DP3cfG4QGC07 for ; Mon, 1 Jul 2019 11:17:06 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=2a00:1450:4864:20::544; helo=mail-ed1-x544.google.com; envelope-from=andy@confluent.io; receiver= Received: from mail-ed1-x544.google.com (mail-ed1-x544.google.com [IPv6:2a00:1450:4864:20::544]) by mx1-he-de.apache.org (ASF Mail Server at mx1-he-de.apache.org) with ESMTPS id EBA197E20A for ; Mon, 1 Jul 2019 11:17:05 +0000 (UTC) Received: by mail-ed1-x544.google.com with SMTP id k8so22846511eds.7 for ; Mon, 01 Jul 2019 04:17:05 -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=5mKxMeUS6e9D7APsXxnuUGMrkuHnYlpGp/nHEEAbdrU=; b=fw55KGmRInrxtgKXOPX/C/BZ78Q8PfI5s2BHoi7WW3JDjV58G/94KWfDfRkbay2QaJ 0+0W1WlEcDi5eAuXLUZsoFKIfTfj4N8+C0m23jmiJsbCu/vbB1MpLl/8fguLYJj6qid6 bik4ybhoq0hyydrcgaQgWpIE647Y4BrXq0TNmeI25fwhhPY5U6iYGx9UMkg8jIW1s1BJ aZLvSkyT0xEf1JSAE9H8RU/DP/z66Q/kARIDWtDwOKrVGXfr578f6WT1SVtFBN9RNQhc OJiQ5crWB/a5TMDVkDa3/QpyhoJ6I1TgItoHiXKGlDjuL4ThyWSYIKIhnoNCYgct3Kbg khfQ== 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=5mKxMeUS6e9D7APsXxnuUGMrkuHnYlpGp/nHEEAbdrU=; b=mBtxdJ5moUS7E1NjzTCaywMGO7vkqPL9Po7mh3Zy18YCvNESAoPXvPL+SgCjULsdvi DUoh9hBKUaVZsaAzUAZEIdqZNqXsFMYeL+97wXkhJwB2vXhHwtT1bAm0wnofov/IxSlS 7IgFe36V7K7s6F9zB/MSb2i52iUBYi18A9s5ppbvyylK0ynq01KMrmouUZkPk8IqsvT2 9l+fNhhtn/Kp+fUQDdIrfhnMFMI4okp88EwuwRtIJn9aWvslaxYwL4zWxolgiCQKGmIp FbDoSXX++y2HQwtnCYVDX6VZ0pYG7Rke6cP6mZRbBdcldPryAkdu+ugwZF9ozOqtYHx7 PWWw== X-Gm-Message-State: APjAAAURPvfhmmfEKH6oVGsYbbhLIz3OcxdSseO8AfQtnxbp8k1w+BHI 04ft3hkYRP1oc32bEZJ8p0ofSIGN/jhqz1RJ0NQCNa3U X-Google-Smtp-Source: APXvYqz88tJn/lanipVPKqw0YuRWhN45Wi6PmR2ONtjvn5KGbb6PRj8cdinaAERTE/lyN+V7x71zkN7frI2Q1HVOLY0= X-Received: by 2002:a05:6402:14cf:: with SMTP id f15mr27705045edx.255.1561979825108; Mon, 01 Jul 2019 04:17:05 -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:16:53 +0100 Message-ID: Subject: Re: [VOTE] KIP-476: Add Java AdminClient interface To: dev Content-Type: multipart/alternative; boundary="0000000000009f53f3058c9cc43c" --0000000000009f53f3058c9cc43c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Done. 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 > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>>> > >> > > > > --0000000000009f53f3058c9cc43c--