Guozhang, Thanks for the KIP. +1 (non-binding)
Best, Bruno On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash <kamal.chandraprak...@gmail.com> wrote: > > Thanks for the KIP! > > LGTM, +1 (non-binding). > > On Wed, Sep 11, 2019 at 3:23 AM Matthias J. Sax <matth...@confluent.io> > wrote: > > > I don't have a strong preference. So I am also fine to deprecate the > > existing methods. Let's see what Jason thinks. > > > > Can you update the KIP to reflect the semantics of the return `Map` (ie, > > does only contain entries for partitions with committed offsets, and > > does not contain `null` values)? > > > > > > +1 (binding) > > > > -Matthias > > > > > > > > > > On 9/10/19 11:53 AM, Guozhang Wang wrote: > > > Hi Jason / Matthias, > > > > > > I understand your concerns now. Just to clarify my main motivation on > > > deprecating the old APIs is not only for aesthetics (confess I personally > > > do have preference on succinct APIs), but to urge people to use the > > batched > > > API for better latency when possible --- as stated in the KIP, my > > > observation is that in practice most callers are actually going to get > > > committed offsets for more than one partitions, and without deprecating > > the > > > old APIs it would be hard for them to realize that the new API does have > > a > > > benefit in performance. > > > > > > This is different from some of the existing APIs though -- e.g., Matthias > > > mentioned about seek / seekToBeginning / seekToEnd, where only seekToXX > > > have plurals and seek only have singulars. We can, of course, make > > seekToXX > > > with plurals as well just like commitSync/Async, but since seeks are > > > non-blocking APIs (they rely on the follow-up polling call to talk to the > > > brokers) either calling it multiple times with one partition each v.s. > > > calling it one time with a plural makes little difference (still, I'd > > argue > > > that today we do not have a same-named function overloaded with both > > types > > > :P) On the other hand, committed, commitSync, offsetsForTimes etc > > blocking > > > calls are all in the form of plurals except > > > > > > * committed > > > * position > > > * partitionsFor > > > > > > My rationale was that 1) for consecutive calls of #position, mostly it > > > would only require a single round-trip to brokers since we are trying to > > > refresh fetching positions for all partitions anyways, and 2) for > > > #partitionsFor, theoretically we could also consider to ask for multiple > > > topics in one call since each blocking call potentially incurs one round > > > trip, but I did not include it in the scope of this KIP only because I > > have > > > not observed too many usage patterns that are commonly calling it > > > consecutively for multiple topics. At the moment, what I truly want to > > > "improve" on is the committed calls, as in many cases I've seen it being > > > called consecutively for multiple topic-partitions. > > > > > > Therefore, I'm still more inclined to deprecate the old APIs so that we > > can > > > enforce people to discover the new batching APIs for efficiency in this > > > KIP. But if you feel that this compatibility is very crucial to maintain > > I > > > could be convinced. > > > > > > > > > Guozhang > > > > > > On Tue, Sep 10, 2019 at 10:18 AM Matthias J. Sax <matth...@confluent.io> > > > wrote: > > > > > >> Thanks for the KIP Guozhang. > > >> > > >>> Another reason is that other functions of KafkaConsumers do not have > > >> those > > >>> overloaded functions to be consistent > > >> > > >> I tend to agree with Jason about keeping the existing methods. Your > > >> argument does not seem to hold. I just checked the `Consumer` API, and > > >> it's mix of overloads: > > >> > > >> Methods only talking `Collections` > > >> > > >> > > >> > > subscribe/assign/commitSync/commitAsyn/pause/resume/offsetsForTimes/beginningOffsets/endOffsets > > >> > > >> Method with overload taking `Collections` or as single value: > > >> > > >> seek/seekToBeginning/seekToEnd > > >> > > >> (those are strictly different methods, but they are semantically > > related) > > >> > > >> Only talking single value: > > >> > > >> position/committed/partitionsFor > > >> > > >> > > >> While you are strictly speaking correct, that there is no method with an > > >> overload for `Collection` and single value, the API mix seems to suggest > > >> that it might actually be worth to have corresponding overloads for all > > >> methods instead of sticking to `Collections` only. > > >> > > >> > > >> > > >> About the return map: I agree that not containing any entry in the map > > >> is better. It's not only consistent with other APIs but it also avoids > > >> potential NPEs. > > >> > > >> > > >> > > >> -Matthias > > >> > > >> > > >> On 9/10/19 10:04 AM, Jason Gustafson wrote: > > >>>> I feel it not worth making committed to have both plurals and > > >> singulars. > > >>> > > >>> Not sure I agree. If we had started with these new APIs from the > > >> beginning, > > >>> that may have been better, but we already have exposed the singular > > APIs > > >>> and users are depending on them. Not sure it's worth breaking > > >> compatibility > > >>> just for aesthetics. > > >>> > > >>> -Jason > > >>> > > >>> On Tue, Sep 10, 2019 at 9:41 AM Guozhang Wang <wangg...@gmail.com> > > >> wrote: > > >>> > > >>>> Thanks Jason! > > >>>> > > >>>> On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson <ja...@confluent.io> > > >>>> wrote: > > >>>> > > >>>>> Hi Guozhang, > > >>>>> > > >>>>> I think the motivation for the new API makes sense. I've wanted > > >> something > > >>>>> like this in the past. That said, do you think there is a substantial > > >>>>> benefit from deprecating the old API? I can still see it being > > >> convenient > > >>>>> in some cases and it's no real cost to maintain. > > >>>>> > > >>>>> > > >>>> That's a good question. > > >>>> > > >>>> Personally I would like to keep a succinct set of APIs out of the box > > >> and > > >>>> let users who want more syntax sugars to add themselves as extended > > >> classes > > >>>> for example (KafkaConsumer is not a final class). > > >>>> Another reason is that other functions of KafkaConsumers do not have > > >> those > > >>>> overloaded functions to be consistent, e.g. we do not have a > > >>>> subscribe(single-topic), pause/resume(single-topic-partition) or > > >>>> seekToBeginning(single-topic-partition). I feel it not worth making > > >>>> committed to have both plurals and singulars. > > >>>> > > >>>> WDYT? > > >>>> > > >>>> > > >>>>> Also, just a minor detail. If the partition has no committed offset, > > >> will > > >>>>> it be present in the map with a null value? > > >>>>> > > >>>>> I looked into the admin client's listConsumerGroupOffsets call when > > >>>> creating the KIP, and to be consistent with that API my intention is > > to > > >> NOT > > >>>> include the entry if a topic-partition does not have committed > > offsets. > > >>>> That said, if we feel returning an entry with null value is better for > > >>>> programmability I can also do that (and will update wiki page to > > >> clarify as > > >>>> well). LMK. > > >>>> > > >>>> > > >>>>> -Jason > > >>>>> > > >>>>> On Tue, Sep 10, 2019 at 6:09 AM Mickael Maison < > > >> mickael.mai...@gmail.com > > >>>>> > > >>>>> wrote: > > >>>>> > > >>>>>> +1 (non-binding), thanks Guozhang > > >>>>>> > > >>>>>> On Tue, Sep 10, 2019 at 1:14 AM Boyang Chen < > > >>>> reluctanthero...@gmail.com> > > >>>>>> wrote: > > >>>>>>> > > >>>>>>> Hey Guozhang, > > >>>>>>> > > >>>>>>> LGTM, +1 (non-binding) > > >>>>>>> > > >>>>>>> On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang <wangg...@gmail.com> > > >>>>> wrote: > > >>>>>>> > > >>>>>>>> Hello folks, > > >>>>>>>> > > >>>>>>>> I've created a new KIP allowing consumer.committed to take a set > > of > > >>>>>>>> partitions instead of just one partition to allow batching effects > > >>>> of > > >>>>>> such > > >>>>>>>> requests (the protocol already allows us to send multiple > > >>>> partitions > > >>>>>> in one > > >>>>>>>> round-trip): > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-520%3A+Add+overloaded+Consumer%23committed+for+batching+partitions > > >>>>>>>> > > >>>>>>>> Since it is a pretty straight-forward KIP, I'm starting the VOTE > > >>>> for > > >>>>>> this > > >>>>>>>> KIP directly. If there are any suggestions about this proposal, > > >>>>> please > > >>>>>> feel > > >>>>>>>> free to share them in this thread. Thank you! > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> -- Guozhang > > >>>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>>> > > >>>> -- > > >>>> -- Guozhang > > >>>> > > >>> > > >> > > >> > > > > > > >
