@Guozhang Wang What do you think?
On Fri, Sep 14, 2018 at 2:39 PM Yishun Guan <gyis...@gmail.com> wrote:
>
> Hi All,
>
> After looking into AdminClient.java and ConsumerClient.java, following
> the original idea, I think some type specific codes are unavoidable
> (we can have a enum class that contain a list of batch-enabled APIs).
> As the compatibility codes that breaks down the batch, we need to
> either map one Builder to multiple Builders, or map one request to
> multiple requests. (I am not an expert, so I would love other's output
> on this.) This will be an extra conditional check before building or
> sending out a request.
>
> From my observation, now a batching optimization for request is only
> needed in KafkaAdminClient (In other words, we want to replace the
> for-loop with a batch request). That limited the scope of the
> optimization, maybe this optimization might seem a little trivial
> compare to the incompatible risk or inconsistency within codes that we
> might face?
>
> If we are not comfortable with making it "ugly and dirty" (or I just
> couldn't enough to come up with a balanced solution) within
> AdminNetworkClient.java and ConsumerNetworkClient.java, we should
> revisit this: https://github.com/apache/kafka/pull/5353 or postpone
> this improvement?
>
> Thanks,
> Yishun
> On Thu, Sep 6, 2018 at 5:22 PM Yishun Guan <gyis...@gmail.com> wrote:
> >
> > Hi Collin and Guozhang,
> >
> > I see. But even if the fall-back logic goes into AdminClient and
> > ConsumerClient, it still have to be somehow type specific right?
> > So either way, there will be api-specific process code somewhere?
> >
> > Thanks,
> > Yishun
> >
> >
> > On Tue, Sep 4, 2018 at 5:46 PM Colin McCabe <co...@cmccabe.xyz> wrote:
> > >
> > > Hi Yishun,
> > >
> > > I agree with Guozhang. NetworkClient is the wrong place to put things
> > > which are specific to a particular message type. NetworkClient should
> > > not have to care what the type of the message is that it is sending.
> > >
> > > Adding type-specific handling is much more "ugly and dirty" than adding
> > > some compatibility code to AdminClient and ConsumerClient. It is true
> > > that there is some code duplication, but I think it will be minimal in
> > > this case.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Tue, Sep 4, 2018, at 13:28, Guozhang Wang wrote:
> > > > Hello Yishun,
> > > >
> > > > I reviewed the latest wiki page, and noticed that the special handling
> > > > logic needs to be in the NetworkClient.
> > > >
> > > > Comparing it with another alternative way, i.e. we add the fall-back
> > > > logic
> > > > in the AdminClient, as well as in the ConsumerClient to capture the
> > > > UnsupportedException and fallback, because the two of them are possibly
> > > > sending FindCoordinatorRequest (though for consumers today we do not
> > > > expect
> > > > it to send for more than one coordinator); personally I think the
> > > > current
> > > > approach is better, but I'd like to hear other people's opinion as well
> > > > (cc'ed Colin, who implemented the AdminClient).
> > > >
> > > >
> > > > Guozhang
> > > >
> > > >
> > > > On Mon, Sep 3, 2018 at 11:57 AM, Yishun Guan <gyis...@gmail.com> wrote:
> > > >
> > > > > Hi Guozhang,
> > > > >
> > > > > Yes, I totally agree with you. Like I said, I think it is an overkill
> > > > > for now, to make so many changes for a small improvement.
> > > > > And like you said, the only way to do this is the "ugly and dirty"
> > > > > way, do you think we should still apply this improvement?
> > > > >
> > > > > I updated a new BuildToList() (method name pending) and add a check
> > > > > condition in the doSend().
> > > > > This is the KIP:https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > 347%3A++Enable+batching+in+FindCoordinatorRequest
> > > > >
> > > > > Let me know what you think.
> > > > >
> > > > > Thanks,
> > > > > Yishun
> > > > > On Sun, Sep 2, 2018 at 10:02 PM Guozhang Wang <wangg...@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > Hi Yishun,
> > > > > >
> > > > > > I was actually not suggesting we should immediately make such
> > > > > > dramatic
> > > > > > change on the AbstractRequest APIs which will affect all requests
> > > > > > types,
> > > > > > just clarifying if it is your intent or not, since your code
> > > > > > snippet in
> > > > > the
> > > > > > KIP has "@Override" :)
> > > > > >
> > > > > > I think an alternative way is to add such a function for for
> > > > > > FindCoordinator only, i.e. besides the overridden `public
> > > > > > FindCoordinatorRequest build(short version)` we can have one more
> > > > > function
> > > > > > (note the function name need to be different since Java type erasure
> > > > > caused
> > > > > > it to not able to differentiate these two otherwise, but we can
> > > > > > consider
> > > > > a
> > > > > > better name: buildMulti is only for illustration)
> > > > > >
> > > > > > public List<FindCoordinatorRequest> buildMulti(short version)
> > > > > >
> > > > > >
> > > > > > It does mean that we now need to special-handle
> > > > > > FindCoordinatorRequestBuilder in all callers from other requests,
> > > > > > which
> > > > > is
> > > > > > also a bit "ugly and dirty", but the change scope may be smaller.
> > > > > > General
> > > > > > changes on the AbstractRequestBuilder could be delayed until we
> > > > > > realize
> > > > > > this is a common usage for some other requests in their newer
> > > > > > versions as
> > > > > > well.
> > > > > >
> > > > > >
> > > > > > Guozhang
> > > > > >
> > > > > >
> > > > > > On Sun, Sep 2, 2018 at 4:10 PM, Yishun Guan <gyis...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Guozhang,
> > > > > > >
> > > > > > > Yes, you are right. I didn't notice T build() is bounded to <T
> > > > > > > extends
> > > > > > > AbstractRequest>.
> > > > > > > I was originally thinking T could be an AbstractedRequest or
> > > > > > > List<>
> > > > > > > but I was wrong. Now the return type has to change from T build()
> > > > > > > to
> > > > > > > List<T> build
> > > > > > > where <T extends AbstractRequest>. As you mentioned,
> > > > > > > this required a change for all the requests, probably need
> > > > > > > a new KIP too, do you think. I will update this KIP accordingly
> > > > > > > first.
> > > > > > >
> > > > > > > However, do you see other benefits of changing the return type for
> > > > > build()?
> > > > > > > The original improvement that we want is this:
> > > > > > > https://issues.apache.org/jira/browse/KAFKA-6788.
> > > > > > > It seems like we have to make a lot of structural changes for this
> > > > > > > small improvement.
> > > > > > > I think changing the return type might benefit the project in the
> > > > > future,
> > > > > > > but I don't know the project enough to say so. I would love to
> > > > > > > keep
> > > > > > > working on this,
> > > > > > > but do you see all these changes are worth for this story,
> > > > > > > and if not, is there another way out?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Yishun
> > > > > > > On Sat, Sep 1, 2018 at 11:04 AM Guozhang Wang <wangg...@gmail.com>
> > > > > wrote:
> > > > > > > >
> > > > > > > > Hello Yishun,
> > > > > > > >
> > > > > > > > Thanks for the updated KIP. I think option 1), i.e. return
> > > > > > > > multiple
> > > > > > > > requests from build() call is okay. Just to clarify: you are
> > > > > > > > going to
> > > > > > > > change `AbstractRequest#build(short version)` signature, and
> > > > > > > > hence
> > > > > all
> > > > > > > > requests need to be updated accordingly, but only
> > > > > > > > FindCoordinator
> > > > > for may
> > > > > > > > return multiple requests in the list, while all others will
> > > > > > > > return
> > > > > > > > singleton list, right?
> > > > > > > >
> > > > > > > >
> > > > > > > > Guozhang
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, Aug 31, 2018 at 10:51 AM, Yishun Guan
> > > > > > > > <gyis...@gmail.com>
> > > > > wrote:
> > > > > > > >
> > > > > > > > > @Guozhang Wang Could you review this again when you have time?
> > > > > Thanks!
> > > > > > > > > -Yishun
> > > > > > > > > On Wed, Aug 29, 2018 at 11:57 AM Yishun Guan
> > > > > > > > > <gyis...@gmail.com>
> > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi, because I have made some significant changes on this
> > > > > > > > > > design,
> > > > > so I
> > > > > > > > > > want to reopen the discussion on this KIP:
> > > > > > > > > > https://cwiki.apache.org/confluence/x/CgZPBQ
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Yishun
> > > > > > > > > > On Thu, Aug 16, 2018 at 5:06 PM Yishun Guan
> > > > > > > > > > <gyis...@gmail.com>
> > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > I see! Thanks!
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> > > > > wangg...@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > > >>
> > > > > > > > > > >> It is not implemented, but should not be hard to do so
> > > > > > > > > > >> (and
> > > > > again
> > > > > > > you
> > > > > > > > > do
> > > > > > > > > > >> NOT have to do that in this KIP, I'm bringing this up so
> > > > > > > > > > >> that
> > > > > you
> > > > > > > can
> > > > > > > > > help
> > > > > > > > > > >> thinking about the process).
> > > > > > > > > > >>
> > > > > > > > > > >> Quoting from Colin's comment:
> > > > > > > > > > >>
> > > > > > > > > > >> "
> > > > > > > > > > >> The pattern is that you would try to send a request for
> > > > > > > > > > >> more
> > > > > than
> > > > > > > one
> > > > > > > > > > >> group, and then you would get an
> > > > > > > > > > >> UnsupportedVersionException
> > > > > > > (nothing
> > > > > > > > > would
> > > > > > > > > > >> be sent on the wire, this is purely internal to the
> > > > > > > > > > >> code).
> > > > > > > > > > >> Then your code would handle the UVE by creating requests
> > > > > > > > > > >> with
> > > > > an
> > > > > > > older
> > > > > > > > > > >> version that only had one group each.
> > > > > > > > > > >> "
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >> Guozhang
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >> On Wed, Aug 15, 2018 at 4:44 PM, Yishun Guan <
> > > > > gyis...@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > > >>
> > > > > > > > > > >> > Hi, I am looking into AdminClient.scala and
> > > > > AdminClient.java,
> > > > > > > and
> > > > > > > > > also
> > > > > > > > > > >> > looking into ApiVersionRequest.java and
> > > > > ApiVersionResponse.java,
> > > > > > > > > but I
> > > > > > > > > > >> > don't see anywhere contains to logic of the one-to-one
> > > > > mapping
> > > > > > > from
> > > > > > > > > version
> > > > > > > > > > >> > to version, am i looking at the right place?
> > > > > > > > > > >> >
> > > > > > > > > > >> > On Mon, Aug 13, 2018 at 1:30 PM Guozhang Wang <
> > > > > > > wangg...@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > > >> >
> > > > > > > > > > >> > > Regarding 3): Today we do not have this logic with
> > > > > > > > > > >> > > the
> > > > > > > existing
> > > > > > > > > client,
> > > > > > > > > > >> > > because defer the decision about the version to use
> > > > > > > > > > >> > > (we
> > > > > always
> > > > > > > > > assume
> > > > > > > > > > >> > that
> > > > > > > > > > >> > > an new versioned request need to be down-converted
> > > > > > > > > > >> > > to a
> > > > > > > single old
> > > > > > > > > > >> > > versioned request: i.e. an one-to-one mapping), but
> > > > > > > > > > >> > > in
> > > > > > > principle,
> > > > > > > > > we
> > > > > > > > > > >> > should
> > > > > > > > > > >> > > be able to modify the client make it work.
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > Again this is not necessarily need to be included in
> > > > > > > > > > >> > > this
> > > > > KIP,
> > > > > > > > > but I'd
> > > > > > > > > > >> > > recommend you to look into AdminClient
> > > > > > > > > > >> > > implementations
> > > > > around
> > > > > > > the
> > > > > > > > > > >> > > ApiVersionRequest / Response and think about how that
> > > > > logic
> > > > > > > can be
> > > > > > > > > > >> > modified
> > > > > > > > > > >> > > in the follow-up PR of this KIP.
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > Guozhang
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > On Mon, Aug 13, 2018 at 12:55 PM, Yishun Guan <
> > > > > > > gyis...@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > > @Guozhang, thank you so much!
> > > > > > > > > > >> > > > 1. I agree, fixed.
> > > > > > > > > > >> > > > 2. Added.
> > > > > > > > > > >> > > > 3. I see, that is something that I haven't think
> > > > > > > > > > >> > > > about.
> > > > > How
> > > > > > > > > does Kafka
> > > > > > > > > > >> > > > handle other api's different version problem now?
> > > > > > > > > > >> > > > So we
> > > > > > > have a
> > > > > > > > > specific
> > > > > > > > > > >> > > > convertor that convect a new version request to a
> > > > > > > > > > >> > > > old
> > > > > > > version
> > > > > > > > > one for
> > > > > > > > > > >> > > each
> > > > > > > > > > >> > > > API (is this what the ApiVersionsRequest supposed
> > > > > > > > > > >> > > > to do,
> > > > > > > does
> > > > > > > > > it only
> > > > > > > > > > >> > > > handle the detection or it should handle the
> > > > > > > > > > >> > > > conversion
> > > > > > > too)?
> > > > > > > > > What will
> > > > > > > > > > >> > > be
> > > > > > > > > > >> > > > the consequence of not having such a transformer
> > > > > > > > > > >> > > > but the
> > > > > > > > > version is
> > > > > > > > > > >> > > > incompatible?
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > Best,
> > > > > > > > > > >> > > > Yishun
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > On Sat, Aug 11, 2018 at 11:27 AM Guozhang Wang <
> > > > > > > > > wangg...@gmail.com>
> > > > > > > > > > >> > > wrote:
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > > Hello Yishun,
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > Thanks for the proposed KIP. I made a pass over
> > > > > > > > > > >> > > > > the
> > > > > wiki
> > > > > > > and
> > > > > > > > > here are
> > > > > > > > > > >> > > > some
> > > > > > > > > > >> > > > > comments:
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > 1. "DESCRIBE_GROUPS_RESPONSE_MEMBER_V0", why we
> > > > > > > > > > >> > > > > need
> > > > > to
> > > > > > > > > encode the
> > > > > > > > > > >> > full
> > > > > > > > > > >> > > > > schema for the "COORDINATOR_GROUPIDS_KEY_NAME"
> > > > > > > > > > >> > > > > field?
> > > > > > > Note it
> > > > > > > > > > >> > includes
> > > > > > > > > > >> > > a
> > > > > > > > > > >> > > > > lot of fields such as member id that is not
> > > > > > > > > > >> > > > > needed for
> > > > > > > this
> > > > > > > > > case. I
> > > > > > > > > > >> > > > think a
> > > > > > > > > > >> > > > > "new ArrayOf(String)" for the group ids should be
> > > > > > > sufficient.
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > 2. "schemaVersions" of the
> > > > > > > > > > >> > > > > "FindCoordinatorRequest"
> > > > > needs
> > > > > > > to
> > > > > > > > > include
> > > > > > > > > > >> > > > > FIND_COORDINATOR_REQUEST_V3 as well.
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > 3. One thing you may need to consider is that,
> > > > > > > > > > >> > > > > in the
> > > > > > > > > adminClient to
> > > > > > > > > > >> > > > handle
> > > > > > > > > > >> > > > > broker compatibility, how to transform a new (v3)
> > > > > request
> > > > > > > to
> > > > > > > > > a bunch
> > > > > > > > > > >> > of
> > > > > > > > > > >> > > > > (v2) requests if it detects the broker is still
> > > > > > > > > > >> > > > > in old
> > > > > > > > > version and
> > > > > > > > > > >> > > hence
> > > > > > > > > > >> > > > > cannot support v3 request (this logic is already
> > > > > > > implemented
> > > > > > > > > via
> > > > > > > > > > >> > > > > ApiVersionsRequest in AdminClient, but may need
> > > > > > > > > > >> > > > > to be
> > > > > > > > > extended to
> > > > > > > > > > >> > > handle
> > > > > > > > > > >> > > > > one-to-many mapping of different versions).
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > This is not sth. that you need to implement
> > > > > > > > > > >> > > > > under this
> > > > > > > KIP,
> > > > > > > > > but I'd
> > > > > > > > > > >> > > > > recommend you think about this earlier than
> > > > > > > > > > >> > > > > later and
> > > > > see
> > > > > > > if
> > > > > > > > > it may
> > > > > > > > > > >> > > > affect
> > > > > > > > > > >> > > > > this proposal.
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > Guozhang
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > On Sat, Aug 11, 2018 at 10:54 AM, Yishun Guan <
> > > > > > > > > gyis...@gmail.com>
> > > > > > > > > > >> > > wrote:
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > > Hi, thank you Ted! I have addressed your
> > > > > > > > > > >> > > > > > comments:
> > > > > > > > > > >> > > > > >
> > > > > > > > > > >> > > > > > 1. Added more descriptions about later
> > > > > > > > > > >> > > > > > optimization.
> > > > > > > > > > >> > > > > > 2. Yes, I will implement the V3 later when
> > > > > > > > > > >> > > > > > this KIP
> > > > > gets
> > > > > > > > > accepted.
> > > > > > > > > > >> > > > > > 3. Fixed.
> > > > > > > > > > >> > > > > >
> > > > > > > > > > >> > > > > > Thanks,
> > > > > > > > > > >> > > > > > Yishun
> > > > > > > > > > >> > > > > >
> > > > > > > > > > >> > > > > > On Fri, Aug 10, 2018 at 3:32 PM Ted Yu <
> > > > > > > yuzhih...@gmail.com
> > > > > > > > > >
> > > > > > > > > > >> > wrote:
> > > > > > > > > > >> > > > > >
> > > > > > > > > > >> > > > > > > bq. this is the foundation of some later
> > > > > > > > > > >> > > > > > > possible
> > > > > > > > > > >> > > > optimizations(enable
> > > > > > > > > > >> > > > > > > batching in *describeConsumerGroups ...*
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > *Can you say more why this change lays the
> > > > > foundation
> > > > > > > for
> > > > > > > > > the
> > > > > > > > > > >> > > future
> > > > > > > > > > >> > > > > > > optimizations ?*
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > *You mentioned **FIND_COORDINATOR_REQUEST_V3
> > > > > > > > > > >> > > > > > > in
> > > > > the
> > > > > > > wiki
> > > > > > > > > but I
> > > > > > > > > > >> > > don't
> > > > > > > > > > >> > > > > see
> > > > > > > > > > >> > > > > > it
> > > > > > > > > > >> > > > > > > in PR.*
> > > > > > > > > > >> > > > > > > *I assume you would add that later.*
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > *Please read your wiki and fix grammatical
> > > > > > > > > > >> > > > > > > error
> > > > > such
> > > > > > > as
> > > > > > > > > the
> > > > > > > > > > >> > > > > following:*
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > bq. that need to be make
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > Thanks
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > On Wed, Aug 8, 2018 at 3:55 PM Yishun Guan <
> > > > > > > > > gyis...@gmail.com>
> > > > > > > > > > >> > > > wrote:
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > > > > Hi all,
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > > > I would like to start a discussion on:
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > > > KIP-347: Enable batching in
> > > > > FindCoordinatorRequest
> > > > > > > > > > >> > > > > > > > https://cwiki.apache.org/confluence/x/CgZPBQ
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > > > Thanks @Guozhang Wang <wangg...@gmail.com>
> > > > > > > > > > >> > > > > > > > for
> > > > > his
> > > > > > > > > help and
> > > > > > > > > > >> > > > > patience!
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > > > Thanks,
> > > > > > > > > > >> > > > > > > > Yishun
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > --
> > > > > > > > > > >> > > > > -- Guozhang
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > --
> > > > > > > > > > >> > > -- Guozhang
> > > > > > > > > > >> > >
> > > > > > > > > > >> >
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >> --
> > > > > > > > > > >> -- Guozhang
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > -- Guozhang
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > -- Guozhang
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > -- Guozhang
