@Guozhang Wang Could you review this again when you have time? Thanks! -Yishun
On Wed, Aug 29, 2018 at 11:57 AM Yishun Guan <gyis...@gmail.com> wrote:
>
> Hi, because I have made some significant changes on this design, so I
> want to reopen the discussion on this KIP:
> https://cwiki.apache.org/confluence/x/CgZPBQ
>
> Thanks,
> Yishun
> On Thu, Aug 16, 2018 at 5:06 PM Yishun Guan <gyis...@gmail.com> wrote:
> >
> > I see! Thanks!
> >
> > On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang <wangg...@gmail.com> wrote:
> >>
> >> It is not implemented, but should not be hard to do so (and again you do
> >> NOT have to do that in this KIP, I'm bringing this up so that you can help
> >> thinking about the process).
> >>
> >> Quoting from Colin's comment:
> >>
> >> "
> >> The pattern is that you would try to send a request for more than one
> >> group, and then you would get an UnsupportedVersionException (nothing would
> >> be sent on the wire, this is purely internal to the code).
> >> Then your code would handle the UVE by creating requests with an older
> >> version that only had one group each.
> >> "
> >>
> >>
> >> Guozhang
> >>
> >>
> >> On Wed, Aug 15, 2018 at 4:44 PM, Yishun Guan <gyis...@gmail.com> wrote:
> >>
> >> > Hi, I am looking into AdminClient.scala and AdminClient.java, and also
> >> > looking into ApiVersionRequest.java and ApiVersionResponse.java, but I
> >> > don't see anywhere contains to logic of the one-to-one mapping from
> >> > version
> >> > to version, am i looking at the right place?
> >> >
> >> > On Mon, Aug 13, 2018 at 1:30 PM Guozhang Wang <wangg...@gmail.com> wrote:
> >> >
> >> > > Regarding 3): Today we do not have this logic with the existing client,
> >> > > because defer the decision about the version to use (we always assume
> >> > that
> >> > > an new versioned request need to be down-converted to a single old
> >> > > versioned request: i.e. an one-to-one mapping), but in principle, we
> >> > should
> >> > > be able to modify the client make it work.
> >> > >
> >> > > Again this is not necessarily need to be included in this KIP, but I'd
> >> > > recommend you to look into AdminClient implementations around the
> >> > > ApiVersionRequest / Response and think about how that logic can be
> >> > modified
> >> > > in the follow-up PR of this KIP.
> >> > >
> >> > >
> >> > >
> >> > > Guozhang
> >> > >
> >> > > On Mon, Aug 13, 2018 at 12:55 PM, Yishun Guan <gyis...@gmail.com>
> >> > > wrote:
> >> > >
> >> > > > @Guozhang, thank you so much!
> >> > > > 1. I agree, fixed.
> >> > > > 2. Added.
> >> > > > 3. I see, that is something that I haven't think about. How does
> >> > > > Kafka
> >> > > > handle other api's different version problem now? So we have a
> >> > > > specific
> >> > > > convertor that convect a new version request to a old version one for
> >> > > each
> >> > > > API (is this what the ApiVersionsRequest supposed to do, does it only
> >> > > > handle the detection or it should handle the conversion too)? What
> >> > > > will
> >> > > be
> >> > > > the consequence of not having such a transformer but the version is
> >> > > > incompatible?
> >> > > >
> >> > > > Best,
> >> > > > Yishun
> >> > > >
> >> > > > On Sat, Aug 11, 2018 at 11:27 AM Guozhang Wang <wangg...@gmail.com>
> >> > > wrote:
> >> > > >
> >> > > > > Hello Yishun,
> >> > > > >
> >> > > > > Thanks for the proposed KIP. I made a pass over the wiki and here
> >> > > > > are
> >> > > > some
> >> > > > > comments:
> >> > > > >
> >> > > > > 1. "DESCRIBE_GROUPS_RESPONSE_MEMBER_V0", why we need to encode the
> >> > full
> >> > > > > schema for the "COORDINATOR_GROUPIDS_KEY_NAME" field? Note it
> >> > includes
> >> > > a
> >> > > > > lot of fields such as member id that is not needed for this case. I
> >> > > > think a
> >> > > > > "new ArrayOf(String)" for the group ids should be sufficient.
> >> > > > >
> >> > > > > 2. "schemaVersions" of the "FindCoordinatorRequest" needs to
> >> > > > > include
> >> > > > > FIND_COORDINATOR_REQUEST_V3 as well.
> >> > > > >
> >> > > > > 3. One thing you may need to consider is that, in the adminClient
> >> > > > > to
> >> > > > handle
> >> > > > > broker compatibility, how to transform a new (v3) request to a
> >> > > > > bunch
> >> > of
> >> > > > > (v2) requests if it detects the broker is still in old version and
> >> > > hence
> >> > > > > cannot support v3 request (this logic is already implemented via
> >> > > > > ApiVersionsRequest in AdminClient, but may need to be extended to
> >> > > handle
> >> > > > > one-to-many mapping of different versions).
> >> > > > >
> >> > > > > This is not sth. that you need to implement under this KIP, but I'd
> >> > > > > recommend you think about this earlier than later and see if it may
> >> > > > affect
> >> > > > > this proposal.
> >> > > > >
> >> > > > >
> >> > > > > Guozhang
> >> > > > >
> >> > > > >
> >> > > > > On Sat, Aug 11, 2018 at 10:54 AM, Yishun Guan <gyis...@gmail.com>
> >> > > wrote:
> >> > > > >
> >> > > > > > Hi, thank you Ted! I have addressed your comments:
> >> > > > > >
> >> > > > > > 1. Added more descriptions about later optimization.
> >> > > > > > 2. Yes, I will implement the V3 later when this KIP gets
> >> > > > > > accepted.
> >> > > > > > 3. Fixed.
> >> > > > > >
> >> > > > > > Thanks,
> >> > > > > > Yishun
> >> > > > > >
> >> > > > > > On Fri, Aug 10, 2018 at 3:32 PM Ted Yu <yuzhih...@gmail.com>
> >> > wrote:
> >> > > > > >
> >> > > > > > > bq. this is the foundation of some later possible
> >> > > > optimizations(enable
> >> > > > > > > batching in *describeConsumerGroups ...*
> >> > > > > > >
> >> > > > > > > *Can you say more why this change lays the foundation for the
> >> > > future
> >> > > > > > > optimizations ?*
> >> > > > > > >
> >> > > > > > > *You mentioned **FIND_COORDINATOR_REQUEST_V3 in the wiki but I
> >> > > don't
> >> > > > > see
> >> > > > > > it
> >> > > > > > > in PR.*
> >> > > > > > > *I assume you would add that later.*
> >> > > > > > >
> >> > > > > > > *Please read your wiki and fix grammatical error such as the
> >> > > > > following:*
> >> > > > > > >
> >> > > > > > > bq. that need to be make
> >> > > > > > >
> >> > > > > > > Thanks
> >> > > > > > >
> >> > > > > > > On Wed, Aug 8, 2018 at 3:55 PM Yishun Guan <gyis...@gmail.com>
> >> > > > wrote:
> >> > > > > > >
> >> > > > > > > > Hi all,
> >> > > > > > > >
> >> > > > > > > > I would like to start a discussion on:
> >> > > > > > > >
> >> > > > > > > > KIP-347: Enable batching in FindCoordinatorRequest
> >> > > > > > > > https://cwiki.apache.org/confluence/x/CgZPBQ
> >> > > > > > > >
> >> > > > > > > > Thanks @Guozhang Wang <wangg...@gmail.com> for his help and
> >> > > > > patience!
> >> > > > > > > >
> >> > > > > > > > Thanks,
> >> > > > > > > > Yishun
> >> > > > > > > >
> >> > > > > > >
> >> > > > > >
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > > --
> >> > > > > -- Guozhang
> >> > > > >
> >> > > >
> >> > >
> >> > >
> >> > >
> >> > > --
> >> > > -- Guozhang
> >> > >
> >> >
> >>
> >>
> >>
> >> --
> >> -- Guozhang
