Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
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 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){ > > 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 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 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 wrote: > > > > > > Thanks for the clarification. I will address this in my KIP. > > > > > > On Fri, Aug 17, 2018, 12:06 PM Guozhang Wang 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 > > 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 > > 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 > > >
Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
Looking at the code for solution #1: } else if (builder.build(version) <https://cwiki.apache.org/confluence/display/KAFKA/builder.build(version)> instanceof List){ 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 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 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 wrote: > > > > Thanks for the clarification. I will address this in my KIP. > > > > On Fri, Aug 17, 2018, 12:06 PM Guozhang Wang 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 > 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 > 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 > >>>> > >>&
Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
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 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 wrote: > > Thanks for the clarification. I will address this in my KIP. > > On Fri, Aug 17, 2018, 12:06 PM Guozhang Wang 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 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 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 wrote: >>>> >>>> > +1 (non-binding) >>>> > >>>> > >>>> > 发件人: Yishun Guan >>>> > 发送时间: 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
Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
Thanks for the clarification. I will address this in my KIP. On Fri, Aug 17, 2018, 12:06 PM Guozhang Wang 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 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 >> 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 wrote: >>> >>> > +1 (non-binding) >>> > >>> > >>> > 发件人: Yishun Guan >>> > 发送时间: 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 >
Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
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 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 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 wrote: >> >> > +1 (non-binding) >> > >> > >> > 发件人: Yishun Guan >> > 发送时间: 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
Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
@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 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 wrote: > > > +1 (non-binding) > > > > > > 发件人: Yishun Guan > > 发送时间: 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 >
Re: 答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
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 wrote: > +1 (non-binding) > > > 发件人: Yishun Guan > 发送时间: 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
答复: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
+1 (non-binding) 发件人: Yishun Guan 发送时间: 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/fd727cc7d5b0956d64255c35d5ed46403c3495a7052ba8ffbc55e084@%3Cdev.kafka.apache.org%3E Thanks everyone for your input! Best, Yishun
Re: [VOTE] KIP-347: Enable batching in FindCoordinatorRequest
+1 On Thu, Aug 16, 2018 at 5:15 PM Yishun Guan wrote: > 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/fd727cc7d5b0956d64255c35d5ed46403c3495a7052ba8ffbc55e084@%3Cdev.kafka.apache.org%3E > > Thanks everyone for your input! > > Best, > Yishun >
[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/fd727cc7d5b0956d64255c35d5ed46403c3495a7052ba8ffbc55e084@%3Cdev.kafka.apache.org%3E Thanks everyone for your input! Best, Yishun