kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Richard Yu <yohan.richard...@gmail.com>
Subject Re: [DISCUSSION] KIP-266: Add TimeoutException to KafkaConsumer#position()
Date Sat, 17 Mar 2018 20:16:48 GMT
Thanks for the advice, Jason

I have modified KIP-266 to include the java doc for committed() and other
blocking methods, and I also
mentioned poll() which will also be bounded. Let me know if there is
anything else. :)

Sincerely, Richard





On Sat, Mar 17, 2018 at 12:00 PM, Jason Gustafson <jason@confluent.io>
wrote:

> Hi Richard,
>
> Thanks for the updates. I'm really glad you picked this up. A couple minor
> comments:
>
> 1. Can you list the full set of new APIs explicitly in the KIP? Currently I
> only see the javadoc for `position()`.
>
> 2. We should consider adding `TimeUnit` to the new methods to avoid unit
> confusion. I know it's inconsistent with the poll() API, but I think it was
> probably a mistake not to include it there, so better not to double down on
> that mistake. And note that we do already have `close(long, TimeUnit)`.
>
> Other than that, I think the current KIP seems reasonable.
>
> Thanks,
> Jason
>
> On Wed, Mar 14, 2018 at 5:00 PM, Richard Yu <yohan.richard.yu@gmail.com>
> wrote:
>
> > Note to all: I have included bounding commitSync() and committed() in
> this
> > KIP.
> >
> > On Sun, Mar 11, 2018 at 5:05 PM, Richard Yu <yohan.richard.yu@gmail.com>
> > wrote:
> >
> > > Hi all,
> > >
> > > I updated the KIP where overloading position() is now the favored
> > approach.
> > > Bounding position() using requestTimeoutMs has been listed as rejected.
> > >
> > > Any thoughts?
> > >
> > > On Tue, Mar 6, 2018 at 6:00 PM, Guozhang Wang <wangguoz@gmail.com>
> > wrote:
> > >
> > >> I agree that adding the overloads is most flexible. But going for that
> > >> direction we'd do that for all the blocking call that I've listed
> above,
> > >> with this timeout value covering the end-to-end waiting time.
> > >>
> > >>
> > >> Guozhang
> > >>
> > >> On Tue, Mar 6, 2018 at 10:02 AM, Ted Yu <yuzhihong@gmail.com> wrote:
> > >>
> > >> > bq. The most flexible option is to add overloads to the consumer
> > >> >
> > >> > This option is flexible.
> > >> >
> > >> > Looking at the tail of SPARK-18057, Spark dev voiced the same
> choice.
> > >> >
> > >> > +1 for adding overload with timeout parameter.
> > >> >
> > >> > Cheers
> > >> >
> > >> > On Mon, Mar 5, 2018 at 2:42 PM, Jason Gustafson <jason@confluent.io
> >
> > >> > wrote:
> > >> >
> > >> > > @Guozhang I probably have suggested all options at some point
or
> > >> another,
> > >> > > including most recently, the current KIP! I was thinking that
> > >> practically
> > >> > > speaking, the request timeout defines how long the user is willing
> > to
> > >> > wait
> > >> > > for a response. The consumer doesn't really have a complex send
> > >> process
> > >> > > like the producer for any of these APIs, so I wasn't sure how
much
> > >> > benefit
> > >> > > there would be from having more granular control over timeouts
(in
> > the
> > >> > end,
> > >> > > KIP-91 just adds a single timeout to control the whole send).
That
> > >> said,
> > >> > it
> > >> > > might indeed be better to avoid overloading the config as you
> > suggest
> > >> > since
> > >> > > at least it avoids inconsistency with the producer's usage.
> > >> > >
> > >> > > The most flexible option is to add overloads to the consumer
so
> that
> > >> > users
> > >> > > can pass the timeout directly. I'm not sure if that is more or
> less
> > >> > > annoying than a new config, but I've found config timeouts a
> little
> > >> > > constraining in practice. For example, I could imagine users
> wanting
> > >> to
> > >> > > wait longer for an offset commit operation than a position lookup;
> > if
> > >> the
> > >> > > latter isn't timely, users can just pause the partition and
> continue
> > >> > > fetching on others. If you cannot commit offsets, however, it
> might
> > be
> > >> > > safer for an application to wait availability of the coordinator
> > than
> > >> > > continuing.
> > >> > >
> > >> > > -Jason
> > >> > >
> > >> > > On Sun, Mar 4, 2018 at 10:14 PM, Guozhang Wang <
> wangguoz@gmail.com>
> > >> > wrote:
> > >> > >
> > >> > > > Hello Richard,
> > >> > > >
> > >> > > > Thanks for the proposed KIP. I have a couple of general
> comments:
> > >> > > >
> > >> > > > 1. I'm not sure if piggy-backing the timeout exception on
the
> > >> > > > existing requestTimeoutMs configured in "request.timeout.ms"
> is a
> > >> good
> > >> > > > idea
> > >> > > > since a) it is a general config that applies for all types
of
> > >> requests,
> > >> > > and
> > >> > > > 2) using it to cover all the phases of an API call, including
> > >> network
> > >> > > round
> > >> > > > trip and potential metadata refresh is shown to not be a
good
> > idea,
> > >> as
> > >> > > > illustrated in KIP-91:
> > >> > > >
> > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > > > 91+Provide+Intuitive+User+Timeouts+in+The+Producer
> > >> > > >
> > >> > > > In fact, I think in KAFKA-4879 which is aimed for the same
issue
> > as
> > >> > > > KAFKA-6608,
> > >> > > > Jason has suggested we use a new config for the API. Maybe
this
> > >> would
> > >> > be
> > >> > > a
> > >> > > > more intuitive manner than reusing the request.timeout.ms
> config.
> > >> > > >
> > >> > > >
> > >> > > > 2. Besides the Consumer.position() call, there are a couple
of
> > more
> > >> > > > blocking calls today that could result in infinite blocking:
> > >> > > > Consumer.commitSync() and Consumer.committed(), should they
be
> > >> > considered
> > >> > > > in this KIP as well?
> > >> > > >
> > >> > > > 3. There are a few other APIs that are today relying on
> > >> > > request.timeout.ms
> > >> > > > already for breaking the infinite blocking, namely
> > >> > > Consumer.partitionFor(),
> > >> > > > Consumer.OffsetAndTimestamp() and Consumer.listTopics(),
if we
> are
> > >> > making
> > >> > > > the other blocking calls to be relying a new config as suggested
> > in
> > >> 1)
> > >> > > > above, should we also change the semantics of these API
> functions
> > >> for
> > >> > > > consistency?
> > >> > > >
> > >> > > >
> > >> > > > Guozhang
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > On Sun, Mar 4, 2018 at 11:13 AM, Richard Yu <
> > >> > yohan.richard.yu@gmail.com>
> > >> > > > wrote:
> > >> > > >
> > >> > > > > Hi all,
> > >> > > > >
> > >> > > > > I would like to discuss a potential change which would
be made
> > to
> > >> > > > > KafkaConsumer:
> > >> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
> > >> > > > action?pageId=75974886
> > >> > > > >
> > >> > > > > Thanks,
> > >> > > > > Richard Yu
> > >> > > > >
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > --
> > >> > > > -- Guozhang
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >>
> > >>
> > >> --
> > >> -- Guozhang
> > >>
> > >
> > >
> >
>

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