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
