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 >
