Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Guozhang Wang <wa...@gmail.com>]

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 <gy...@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 <gy...@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 <gy...@gmail.com> wrote:
> > >
> > > I see! Thanks!
> > >
> > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <wa...@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 <gy...@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 <wa...@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 <gy...@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 <
> wangguoz@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 <
> gyishun@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 <yuzhihong@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 <
> gyishun@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 <wa...@gmail.com> for his
> help and
> > >> > > > > patience!
> > >> > > > > > > >
> > >> > > > > > > > Thanks,
> > >> > > > > > > > Yishun
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > > --
> > >> > > > > -- Guozhang
> > >> > > > >
> > >> > > >
> > >> > >
> > >> > >
> > >> > >
> > >> > > --
> > >> > > -- Guozhang
> > >> > >
> > >> >
> > >>
> > >>
> > >>
> > >> --
> > >> -- Guozhang
>



-- 
-- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Guozhang Wang <wa...@gmail.com>]

Hello Yishun,

I looked further into the change needed on NetworkClient, comparing them
with the changes on AdminClient / ConsumerNetworkClient, and I think it is
indeed a quite large change needed on the latter two classes if we do not
want to touch on NetworkClient.

Colin's argument against having the changes on the NetworkClient is
primarily that the NetworkClient should be ideally only be responsible for
sending request out and receiving responses, and the current implementation
already leaked a lot of the request-type related logic which makes it quite
complicated, so further adding the conversion logic on it will make things
worse. While I agree with the argument, I also feel that the changes on the
higher-level classes (AdminClient and ConsumerNetworkClient) is more
intrusive than we thought originally, which makes this optimization on the
admin client less "cost-effective".

If we want to just make this optimization on AdminClient I'm still fine
with it (in Consumer since we are only going to ask for the coordinator of
a single consumer always), or if you want to postpone this KIP that's also
okay. Personally I felt guilty to let you dive into such a big scoped task
without careful thinking beforehand than suggesting you to start with a
much smaller task to work on.

In case you want to find a smaller JIRA:
https://issues.apache.org/jira/issues/?jql=project%20%3D%20KAFKA%20AND%20resolution%20%3D%20Unresolved%20AND%20labels%20in%20(%22newbie%2B%2B%22%2C%20newbie)%20ORDER%20BY%20assignee%20DESC%2C%20priority%20DESC


Guozhang


On Mon, Sep 17, 2018 at 4:50 PM, Yishun Guan <gy...@gmail.com> wrote:

> @Guozhang Wang What do you think?
> On Fri, Sep 14, 2018 at 2:39 PM Yishun Guan <gy...@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 <gy...@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 <gy...@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 <
> wangguoz@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 <gy...@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 <
> wangguoz@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 <
> gyishun@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 <
> gyishun@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 <
> gyishun@gmail.com>
> > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > I see! Thanks!
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> > > > > > wangguoz@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 <
> > > > > > gyishun@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 <
> > > > > > > > wangguoz@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 <
> > > > > > > > gyishun@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 <
> > > > > > > > > > wangguoz@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 <
> > > > > > > > > > gyishun@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 <
> > > > > > > > yuzhihong@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 <
> > > > > > > > > > gyishun@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 <
> wangguoz@gmail.com> for
> > > > > > his
> > > > > > > > > > help and
> > > > > > > > > > > >> > > > > patience!
> > > > > > > > > > > >> > > > > > > >
> > > > > > > > > > > >> > > > > > > > Thanks,
> > > > > > > > > > > >> > > > > > > > Yishun
> > > > > > > > > > > >> > > > > > > >
> > > > > > > > > > > >> > > > > > >
> > > > > > > > > > > >> > > > > >
> > > > > > > > > > > >> > > > >
> > > > > > > > > > > >> > > > >
> > > > > > > > > > > >> > > > >
> > > > > > > > > > > >> > > > > --
> > > > > > > > > > > >> > > > > -- Guozhang
> > > > > > > > > > > >> > > > >
> > > > > > > > > > > >> > > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > > --
> > > > > > > > > > > >> > > -- Guozhang
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> >
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >> --
> > > > > > > > > > > >> -- Guozhang
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > -- Guozhang
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > -- Guozhang
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > -- Guozhang
>



-- 
-- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Yishun Guan <gy...@gmail.com>]

@Guozhang Wang What do you think?
On Fri, Sep 14, 2018 at 2:39 PM Yishun Guan <gy...@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 <gy...@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 <gy...@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 <wa...@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 <gy...@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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com>
> > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > I see! Thanks!
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> > > > > wangguoz@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 <
> > > > > gyishun@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 <
> > > > > > > wangguoz@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 <
> > > > > > > gyishun@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 <
> > > > > > > > > wangguoz@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 <
> > > > > > > > > gyishun@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 <
> > > > > > > yuzhihong@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 <
> > > > > > > > > gyishun@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 <wa...@gmail.com> for
> > > > > his
> > > > > > > > > help and
> > > > > > > > > > >> > > > > patience!
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > > > Thanks,
> > > > > > > > > > >> > > > > > > > Yishun
> > > > > > > > > > >> > > > > > > >
> > > > > > > > > > >> > > > > > >
> > > > > > > > > > >> > > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > > > --
> > > > > > > > > > >> > > > > -- Guozhang
> > > > > > > > > > >> > > > >
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > --
> > > > > > > > > > >> > > -- Guozhang
> > > > > > > > > > >> > >
> > > > > > > > > > >> >
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >> --
> > > > > > > > > > >> -- Guozhang
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > -- Guozhang
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > -- Guozhang
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > -- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Yishun Guan <gy...@gmail.com>]

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 <gy...@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 <gy...@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 <wa...@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 <gy...@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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com>
> > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > I see! Thanks!
> > > > > > > > > >
> > > > > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> > > > wangguoz@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 <
> > > > gyishun@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 <
> > > > > > wangguoz@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 <
> > > > > > gyishun@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 <
> > > > > > > > wangguoz@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 <
> > > > > > > > gyishun@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 <
> > > > > > yuzhihong@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 <
> > > > > > > > gyishun@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 <wa...@gmail.com> for
> > > > his
> > > > > > > > help and
> > > > > > > > > >> > > > > patience!
> > > > > > > > > >> > > > > > > >
> > > > > > > > > >> > > > > > > > Thanks,
> > > > > > > > > >> > > > > > > > Yishun
> > > > > > > > > >> > > > > > > >
> > > > > > > > > >> > > > > > >
> > > > > > > > > >> > > > > >
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > --
> > > > > > > > > >> > > > > -- Guozhang
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > >
> > > > > > > > > >> > >
> > > > > > > > > >> > >
> > > > > > > > > >> > > --
> > > > > > > > > >> > > -- Guozhang
> > > > > > > > > >> > >
> > > > > > > > > >> >
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> --
> > > > > > > > > >> -- Guozhang
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > -- Guozhang
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > -- Guozhang
> > > >
> > >
> > >
> > >
> > > --
> > > -- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Yishun Guan <gy...@gmail.com>]

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 <gy...@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 <wa...@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 <gy...@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 <wa...@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 <gyishun@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 <
gyishun@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 <
gyishun@gmail.com>
> > > > > wrote:
> > > > > > > > >
> > > > > > > > > I see! Thanks!
> > > > > > > > >
> > > > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> > > wangguoz@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 <
> > > gyishun@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 <
> > > > > wangguoz@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 <
> > > > > gyishun@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 <
> > > > > > > wangguoz@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 <
> > > > > > > gyishun@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 <
> > > > > yuzhihong@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 <
> > > > > > > gyishun@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 <wa...@gmail.com>
for
> > > his
> > > > > > > help and
> > > > > > > > >> > > > > patience!
> > > > > > > > >> > > > > > > >
> > > > > > > > >> > > > > > > > Thanks,
> > > > > > > > >> > > > > > > > Yishun
> > > > > > > > >> > > > > > > >
> > > > > > > > >> > > > > > >
> > > > > > > > >> > > > > >
> > > > > > > > >> > > > >
> > > > > > > > >> > > > >
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > --
> > > > > > > > >> > > > > -- Guozhang
> > > > > > > > >> > > > >
> > > > > > > > >> > > >
> > > > > > > > >> > >
> > > > > > > > >> > >
> > > > > > > > >> > >
> > > > > > > > >> > > --
> > > > > > > > >> > > -- Guozhang
> > > > > > > > >> > >
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> --
> > > > > > > > >> -- Guozhang
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > -- Guozhang
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > -- Guozhang
> > >
> >
> >
> >
> > --
> > -- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Colin McCabe <co...@cmccabe.xyz>]

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 <gy...@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 <wa...@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 <gy...@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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com>
> > > > wrote:
> > > > > > > >
> > > > > > > > I see! Thanks!
> > > > > > > >
> > > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> > wangguoz@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 <
> > gyishun@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 <
> > > > wangguoz@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 <
> > > > gyishun@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 <
> > > > > > wangguoz@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 <
> > > > > > gyishun@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 <
> > > > yuzhihong@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 <
> > > > > > gyishun@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 <wa...@gmail.com> for
> > his
> > > > > > help and
> > > > > > > >> > > > > patience!
> > > > > > > >> > > > > > > >
> > > > > > > >> > > > > > > > Thanks,
> > > > > > > >> > > > > > > > Yishun
> > > > > > > >> > > > > > > >
> > > > > > > >> > > > > > >
> > > > > > > >> > > > > >
> > > > > > > >> > > > >
> > > > > > > >> > > > >
> > > > > > > >> > > > >
> > > > > > > >> > > > > --
> > > > > > > >> > > > > -- Guozhang
> > > > > > > >> > > > >
> > > > > > > >> > > >
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >> > > --
> > > > > > > >> > > -- Guozhang
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> --
> > > > > > > >> -- Guozhang
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > -- Guozhang
> > > >
> > >
> > >
> > >
> > > --
> > > -- Guozhang
> >
> 
> 
> 
> -- 
> -- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Guozhang Wang <wa...@gmail.com>]

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 <gy...@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 <wa...@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 <gy...@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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com>
> > > wrote:
> > > > > > >
> > > > > > > I see! Thanks!
> > > > > > >
> > > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <
> wangguoz@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 <
> gyishun@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 <
> > > wangguoz@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 <
> > > gyishun@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 <
> > > > > wangguoz@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 <
> > > > > gyishun@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 <
> > > yuzhihong@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 <
> > > > > gyishun@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 <wa...@gmail.com> for
> his
> > > > > help and
> > > > > > >> > > > > patience!
> > > > > > >> > > > > > > >
> > > > > > >> > > > > > > > Thanks,
> > > > > > >> > > > > > > > Yishun
> > > > > > >> > > > > > > >
> > > > > > >> > > > > > >
> > > > > > >> > > > > >
> > > > > > >> > > > >
> > > > > > >> > > > >
> > > > > > >> > > > >
> > > > > > >> > > > > --
> > > > > > >> > > > > -- Guozhang
> > > > > > >> > > > >
> > > > > > >> > > >
> > > > > > >> > >
> > > > > > >> > >
> > > > > > >> > >
> > > > > > >> > > --
> > > > > > >> > > -- Guozhang
> > > > > > >> > >
> > > > > > >> >
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >> --
> > > > > > >> -- Guozhang
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > -- Guozhang
> > >
> >
> >
> >
> > --
> > -- Guozhang
>



-- 
-- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Yishun Guan <gy...@gmail.com>]

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 <wa...@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 <gy...@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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com>
> > wrote:
> > > > > >
> > > > > > I see! Thanks!
> > > > > >
> > > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <wa...@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 <gy...@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 <
> > wangguoz@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 <
> > gyishun@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 <
> > > > wangguoz@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 <
> > > > gyishun@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 <
> > yuzhihong@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 <
> > > > gyishun@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 <wa...@gmail.com> for his
> > > > help and
> > > > > >> > > > > patience!
> > > > > >> > > > > > > >
> > > > > >> > > > > > > > Thanks,
> > > > > >> > > > > > > > Yishun
> > > > > >> > > > > > > >
> > > > > >> > > > > > >
> > > > > >> > > > > >
> > > > > >> > > > >
> > > > > >> > > > >
> > > > > >> > > > >
> > > > > >> > > > > --
> > > > > >> > > > > -- Guozhang
> > > > > >> > > > >
> > > > > >> > > >
> > > > > >> > >
> > > > > >> > >
> > > > > >> > >
> > > > > >> > > --
> > > > > >> > > -- Guozhang
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> --
> > > > > >> -- Guozhang
> > > >
> > >
> > >
> > >
> > > --
> > > -- Guozhang
> >
>
>
>
> --
> -- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Guozhang Wang <wa...@gmail.com>]

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 <gy...@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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com>
> wrote:
> > > > >
> > > > > I see! Thanks!
> > > > >
> > > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <wa...@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 <gy...@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 <
> wangguoz@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 <
> gyishun@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 <
> > > wangguoz@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 <
> > > gyishun@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 <
> yuzhihong@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 <
> > > gyishun@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 <wa...@gmail.com> for his
> > > help and
> > > > >> > > > > patience!
> > > > >> > > > > > > >
> > > > >> > > > > > > > Thanks,
> > > > >> > > > > > > > Yishun
> > > > >> > > > > > > >
> > > > >> > > > > > >
> > > > >> > > > > >
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > > --
> > > > >> > > > > -- Guozhang
> > > > >> > > > >
> > > > >> > > >
> > > > >> > >
> > > > >> > >
> > > > >> > >
> > > > >> > > --
> > > > >> > > -- Guozhang
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >>
> > > > >>
> > > > >> --
> > > > >> -- Guozhang
> > >
> >
> >
> >
> > --
> > -- Guozhang
>



-- 
-- Guozhang

Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest

[Yishun Guan <gy...@gmail.com>]

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 <wa...@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 <gy...@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 <gy...@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 <gy...@gmail.com> wrote:
> > > >
> > > > I see! Thanks!
> > > >
> > > > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <wa...@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 <gy...@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 <wa...@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 <gy...@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 <
> > wangguoz@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 <
> > gyishun@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 <yuzhihong@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 <
> > gyishun@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 <wa...@gmail.com> for his
> > help and
> > > >> > > > > patience!
> > > >> > > > > > > >
> > > >> > > > > > > > Thanks,
> > > >> > > > > > > > Yishun
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > --
> > > >> > > > > -- Guozhang
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> > >
> > > >> > >
> > > >> > > --
> > > >> > > -- Guozhang
> > > >> > >
> > > >> >
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> -- Guozhang
> >
>
>
>
> --
> -- Guozhang