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 <ja...@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...@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...@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 <wangg...@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 <yuzhih...@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 <ja...@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 < > wangg...@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...@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 > > >> > > > > > > > > >
