Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-13 Thread Guozhang Wang
Thanks everyone for your votes.

+1 from myself (binding).

I'm closing this voting thread now with the following tally:

binding +1: 4 (Bill, Matthias, Jason, Guozhang)
non-binding +1: 6 (Boyang, Mickael, Kamal, Bruno, Manna, Tom)


Thanks,
Guozhang


On Thu, Sep 12, 2019 at 11:02 AM Jason Gustafson  wrote:

> Hi Guozhang,
>
> That's a fair point. I think originally `committed` just hit the local
> cache. For EOS, we changed it to send a request on every call since the
> updated value came from outside the consumer. Given that, I think it is
> reasonable to push users toward the batch API.
>
> +1
>
> -Jason
>
> On Thu, Sep 12, 2019 at 2:04 AM Tom Bentley  wrote:
>
> > +1 (non-binding).
> >
> > Thanks!
> >
> > On Thu, Sep 12, 2019 at 9:58 AM M. Manna  wrote:
> >
> > > Gushing,
> > >
> > > Good kip. +1 (binding) from me.
> > >
> > > Thanks,
> > > MAnna
> > >
> > > On Thu, 12 Sep 2019 at 09:55, Bruno Cadonna 
> wrote:
> > >
> > > > Guozhang, Thanks for the KIP.
> > > >
> > > > +1 (non-binding)
> > > >
> > > > Best,
> > > > Bruno
> > > >
> > > > On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash
> > > >  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 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-12 Thread Jason Gustafson
Hi Guozhang,

That's a fair point. I think originally `committed` just hit the local
cache. For EOS, we changed it to send a request on every call since the
updated value came from outside the consumer. Given that, I think it is
reasonable to push users toward the batch API.

+1

-Jason

On Thu, Sep 12, 2019 at 2:04 AM Tom Bentley  wrote:

> +1 (non-binding).
>
> Thanks!
>
> On Thu, Sep 12, 2019 at 9:58 AM M. Manna  wrote:
>
> > Gushing,
> >
> > Good kip. +1 (binding) from me.
> >
> > Thanks,
> > MAnna
> >
> > On Thu, 12 Sep 2019 at 09:55, Bruno Cadonna  wrote:
> >
> > > Guozhang, Thanks for the KIP.
> > >
> > > +1 (non-binding)
> > >
> > > Best,
> > > Bruno
> > >
> > > On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash
> > >  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 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-12 Thread Tom Bentley
+1 (non-binding).

Thanks!

On Thu, Sep 12, 2019 at 9:58 AM M. Manna  wrote:

> Gushing,
>
> Good kip. +1 (binding) from me.
>
> Thanks,
> MAnna
>
> On Thu, 12 Sep 2019 at 09:55, Bruno Cadonna  wrote:
>
> > Guozhang, Thanks for the KIP.
> >
> > +1 (non-binding)
> >
> > Best,
> > Bruno
> >
> > On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash
> >  wrote:
> > >
> > > Thanks for the KIP!
> > >
> > > LGTM, +1 (non-binding).
> > >
> > > On Wed, Sep 11, 2019 at 3:23 AM Matthias J. Sax  >
> > > 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 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-12 Thread M. Manna
Gushing,

Good kip. +1 (binding) from me.

Thanks,
MAnna

On Thu, 12 Sep 2019 at 09:55, Bruno Cadonna  wrote:

> Guozhang, Thanks for the KIP.
>
> +1 (non-binding)
>
> Best,
> Bruno
>
> On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash
>  wrote:
> >
> > Thanks for the KIP!
> >
> > LGTM, +1 (non-binding).
> >
> > On Wed, Sep 11, 2019 at 3:23 AM Matthias J. Sax 
> > 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. 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-12 Thread Bruno Cadonna
Guozhang, Thanks for the KIP.

+1 (non-binding)

Best,
Bruno

On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash
 wrote:
>
> Thanks for the KIP!
>
> LGTM, +1 (non-binding).
>
> On Wed, Sep 11, 2019 at 3:23 AM Matthias J. Sax 
> 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 
> > > 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 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-11 Thread Kamal Chandraprakash
Thanks for the KIP!

LGTM, +1 (non-binding).

On Wed, Sep 11, 2019 at 3:23 AM Matthias J. Sax 
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 
> > 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 
> >> wrote:
> >>>
>  Thanks Jason!
> 
>  On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson 
>  wrote:
> 
> > Hi Guozhang,
> >
> > I think the 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Matthias J. Sax
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 
> 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 
>> wrote:
>>>
 Thanks Jason!

 On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson 
 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 

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Guozhang Wang
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 
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 
> wrote:
> >
> >> Thanks Jason!
> >>
> >> On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson 
> >> 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.

Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Matthias J. Sax
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  wrote:
> 
>> Thanks Jason!
>>
>> On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson 
>> 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 >>
>>> 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 
>>> 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
>>
> 



signature.asc
Description: OpenPGP digital signature


Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Jason Gustafson
>  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  wrote:

> Thanks Jason!
>
> On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson 
> 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  >
> > 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 
> > 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
>


Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Guozhang Wang
Thanks Jason!

On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson  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 
> wrote:
>
> > +1 (non-binding), thanks Guozhang
> >
> > On Tue, Sep 10, 2019 at 1:14 AM Boyang Chen 
> > wrote:
> > >
> > > Hey Guozhang,
> > >
> > > LGTM, +1 (non-binding)
> > >
> > > On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang 
> 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


Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Jason Gustafson
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.

Also, just a minor detail. If the partition has no committed offset, will
it be present in the map with a null value?

-Jason

On Tue, Sep 10, 2019 at 6:09 AM Mickael Maison 
wrote:

> +1 (non-binding), thanks Guozhang
>
> On Tue, Sep 10, 2019 at 1:14 AM Boyang Chen 
> wrote:
> >
> > Hey Guozhang,
> >
> > LGTM, +1 (non-binding)
> >
> > On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang  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
> > >
>


Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Bill Bejeck
Thanks for the KIP Guozhang.

+1 (binding)

-Bill

On Tue, Sep 10, 2019 at 9:09 AM Mickael Maison 
wrote:

> +1 (non-binding), thanks Guozhang
>
> On Tue, Sep 10, 2019 at 1:14 AM Boyang Chen 
> wrote:
> >
> > Hey Guozhang,
> >
> > LGTM, +1 (non-binding)
> >
> > On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang  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
> > >
>


Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-10 Thread Mickael Maison
+1 (non-binding), thanks Guozhang

On Tue, Sep 10, 2019 at 1:14 AM Boyang Chen  wrote:
>
> Hey Guozhang,
>
> LGTM, +1 (non-binding)
>
> On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang  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
> >


Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions

2019-09-09 Thread Boyang Chen
Hey Guozhang,

LGTM, +1 (non-binding)

On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang  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
>