kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tom Bentley <tbent...@redhat.com>
Subject Re: [VOTE] KIP-520: Augment Consumer.committed(partition) to allow multiple partitions
Date Thu, 12 Sep 2019 09:03:40 GMT
+1 (non-binding).

Thanks!

On Thu, Sep 12, 2019 at 9:58 AM M. Manna <manmedia@gmail.com> wrote:

> Gushing,
>
> Good kip. +1 (binding) from me.
>
> Thanks,
> MAnna
>
> On Thu, 12 Sep 2019 at 09:55, Bruno Cadonna <bruno@confluent.io> wrote:
>
> > Guozhang, Thanks for the KIP.
> >
> > +1 (non-binding)
> >
> > Best,
> > Bruno
> >
> > On Wed, Sep 11, 2019 at 9:17 AM Kamal Chandraprakash
> > <kamal.chandraprakash@gmail.com> wrote:
> > >
> > > Thanks for the KIP!
> > >
> > > LGTM, +1 (non-binding).
> > >
> > > On Wed, Sep 11, 2019 at 3:23 AM Matthias J. Sax <matthias@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 <
> > matthias@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 <
> wangguoz@gmail.com>
> > > > >> wrote:
> > > > >>>
> > > > >>>> Thanks Jason!
> > > > >>>>
> > > > >>>> On Tue, Sep 10, 2019 at 9:07 AM Jason Gustafson <
> > jason@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.maison@gmail.com
> > > > >>>>>
> > > > >>>>> wrote:
> > > > >>>>>
> > > > >>>>>> +1 (non-binding), thanks Guozhang
> > > > >>>>>>
> > > > >>>>>> On Tue, Sep 10, 2019 at 1:14 AM Boyang Chen <
> > > > >>>> reluctanthero104@gmail.com>
> > > > >>>>>> wrote:
> > > > >>>>>>>
> > > > >>>>>>> Hey Guozhang,
> > > > >>>>>>>
> > > > >>>>>>> LGTM, +1 (non-binding)
> > > > >>>>>>>
> > > > >>>>>>> On Mon, Sep 9, 2019 at 5:07 PM Guozhang Wang
<
> > wangguoz@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
> > > > >>>>
> > > > >>>
> > > > >>
> > > > >>
> > > > >
> > > >
> > > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message