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