kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yishun Guan <gyis...@gmail.com>
Subject Re: [DISCUSS] KIP-347: Enable batching in FindCoordinatorRequest
Date Fri, 14 Sep 2018 21:39:27 GMT
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 <gyishun@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 <colin@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 <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
View raw message