kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guozhang Wang <wangg...@gmail.com>
Subject Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest
Date Tue, 04 Sep 2018 20:28:08 GMT
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 <gyishun@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 <gyishun@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message