Hi Ted, you are right! I fixed it to List<?>. I would prefer the first solution because all the logics will be added on NetworkClient.java, and changing the current AbstractRequest class seems a little unnecessary.
Thanks Yishun On Mon, Aug 27, 2018 at 4:57 PM Ted Yu <yuzhih...@gmail.com> wrote: > > Looking at the code for solution #1: > } else if (builder.build(version) > <https://cwiki.apache.org/confluence/display/KAFKA/builder.build(version)> > instanceof List<AbstractRequest>){ > > wouldn't AbstractRequest be gone due to type erasure ? > > Which solution do you favor ? > > Cheers > > On Mon, Aug 27, 2018 at 4:20 PM Yishun Guan <gyis...@gmail.com> wrote: > > > Sorry for the delay, I have been struggling to come up with a nice > > solution: > > > > I have updated the KIP: > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-347%3A++Enable+batching+in+FindCoordinatorRequest > > > > In summary, to solve the question Guozhang raised: > > > > "One tricky question is, how do we know if a higher version API has a > > batching optimization... > > > > a) One solution is to let the request's builder.build() return either > > ONE request or a LIST of requests. This is backward compatible. We can > > have a list of one single element. > > > > b) An alternative solution is to add extra fields in > > AbstractRequest.java including Boolean isBatchingEnable() and > > List<AbstractRequest> buildFromBatch(). This way will decouple the two > > different build functions. > > > > Then we update the send logic in doSend() correspondingly." > > > > > > You can read about these solutions in more details in this KIP. > > > > Thanks, > > Yishun > > > > On Fri, Aug 17, 2018 at 12:12 PM Yishun Guan <gyis...@gmail.com> wrote: > > > > > > Thanks for the clarification. I will address this in my KIP. > > > > > > On Fri, Aug 17, 2018, 12:06 PM Guozhang Wang <wangg...@gmail.com> wrote: > > >> > > >> Today we do have logic for auto down-conversion, but it is assuming a > > one-to-one mapping. The actual logic is here: > > >> > > >> > > https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L775 > > >> > > >> As you can see, NetworkClient maintains a "apiVersions" map that keeps > > which node supports which version. And then when sending the request to the > > node it will use this map to form the supported version of the request. > > >> > > >> But current logic do not consider that we may need multiple lower > > versioned requests to substitute a single higher versioned request, and > > that would be the logic your PR need to address. > > >> > > >> > > >> Guozhang > > >> > > >> On Fri, Aug 17, 2018 at 11:59 AM, Yishun Guan <gyis...@gmail.com> > > wrote: > > >>> > > >>> @Guozhang Wang One thing that I remain confused about (and forgive me > > if you have explained this to me before), is that if we don't have any > > transformation helpers (between versions) implemented before, how do we > > deal with other incompatibility issues when we try to update requests and > > bump up their versions? Or we never have this problem until this version > > update and now that's why we need to implement a converter from V3 to V2? > > >>> > > >>> On Fri, Aug 17, 2018 at 10:18 AM Guozhang Wang <wangg...@gmail.com> > > wrote: > > >>>> > > >>>> Yishun, some more comments: > > >>>> > > >>>> 1. "All the coordinator ids " + "for this request": it should be "all > > the > > >>>> requested group ids looking for their coordinators" right? > > >>>> > > >>>> 2. I just thought about this a bit more, regarding "*Compatibility > > issues > > >>>> between old and new versions need to be considered, we should think > > about > > >>>> how to convert requests from a newer version to a old version."* > > >>>> > > >>>> > > >>>> One thing I realized is that for FindCoordinatorRequest, today both > > >>>> consumer / admin client would need it. I.e. to complete the KIP for > > >>>> compatibility, you'll have to implement this function along with this > > KIP, > > >>>> since otherwise consumer talking to an old broker will fail to. > > >>>> > > >>>> So I'd suggest you update the `Compatibility` section with a detailed > > >>>> proposal on how to let new versioned clients to talk to old versioned > > >>>> brokers. We've talked about some high-level implementation guidelines > > in > > >>>> the DISCUSS thread, which you can try it out and see if it works: > > i.e. by > > >>>> starting a Kafka broker with version 2.0, and then starting a consumer > > >>>> client with trunk (it will have a new version), and the added logic > > should > > >>>> make sure the consumer still proceeds normally with the compatibility > > logic > > >>>> that we are going to add. > > >>>> > > >>>> > > >>>> Guozhang > > >>>> > > >>>> On Thu, Aug 16, 2018 at 5:46 PM, Hu Xi <huxi...@hotmail.com> wrote: > > >>>> > > >>>> > +1 (non-binding) > > >>>> > > > >>>> > ________________________________ > > >>>> > 发件人: Yishun Guan <gyis...@gmail.com> > > >>>> > 发送时间: 2018年8月17日 8:14 > > >>>> > 收件人: dev@kafka.apache.org > > >>>> > 主题: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest > > >>>> > > > >>>> > Hi all, > > >>>> > > > >>>> > I want to start a vote on this KIP: > > >>>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >>>> > 347%3A++Enable+batching+in+FindCoordinatorRequest > > >>>> > > > >>>> > Here is the discussion thread: > > >>>> > https://lists.apache.org/thread.html/fd727cc7d5b0956d64255c35d5ed46 > > >>>> > 403c3495a7052ba8ffbc55e084@%3Cdev.kafka.apache.org%3E > > >>>> > > > >>>> > Thanks everyone for your input! > > >>>> > > > >>>> > Best, > > >>>> > Yishun > > >>>> > > > >>>> > > >>>> > > >>>> > > >>>> -- > > >>>> -- Guozhang > > >> > > >> > > >> > > >> > > >> -- > > >> -- Guozhang > >
