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
