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 <wangg...@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 <gyis...@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 <wangg...@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 <gyis...@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 <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 > > > > > > > > > > > > > > > > -- > > > -- Guozhang > > > > > > -- > -- Guozhang
