Seems reasonable to me. Ismael
On Fri, Jan 6, 2017 at 11:19 AM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Ismael, > > Thank you, I have fixed the javadoc typo. > > Producers use a default close timeout of Long.MAX_VALUE, but with a default > request timeout of 30 seconds, close would complete in ~30 seconds. The > consumer's 5 minutes request timeout is clearly too long as the default > close timeout, but if that is improved in a future release as Ismael > suggested, perhaps that could become 30 seconds to be consistent with > producer request timeout. And then we could make the consumer's default > close timeout use request timeout as the upper bound. So shall we use > hard-coded 30 second timeout for now, but change to use request timeout > when the default request timeout is reduced? > > On Fri, Jan 6, 2017 at 10:22 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Hi, > > > > I am definitely in favour of aiming for good defaults. One concern I have > > is that `30 seconds` is a hardcoded value and not based on any config. It > > is true that users can call the `close` method that takes an explicit > > timeout if necessary, but it would be nice if the no-args `close` would > use > > a timeout inferred from an existing config. From that perspective, I > liked > > Rajini's idea to rely on request timeout. The issue is that the request > > timeout is a bit too large by default (5 minutes) due to some > > implementation details (it would be great to improve this in a future > > release). > > > > Rajini, a typo in the the javadoc for the consumer close method in the > KIP: > > it says "force close the producer". > > > > Ismael > > > > On Fri, Jan 6, 2017 at 2:42 AM, Jason Gustafson <ja...@confluent.io> > > wrote: > > > > > Hey Gwen, > > > > > > I'm not super strong on this, but I think the case for a longer timeout > > as > > > the default behavior is weaker for the consumer. For the producer, it > > means > > > we might lose messages that the application tried to send. For the > > > consumer, it means we might lose offset commits, which means duplicates > > > later on. But if we're not attempting coordinator rediscovery on > > connection > > > failures or retrying offset commits anyway, does the extra time help > that > > > much? I'm not sure, but I'd rather have a reasonable bound on the > > shutdown > > > time as the default behavior and let users who want to wait longer > > provide > > > their own timeout. I've seen a few too many cases in the mail lists > where > > > users complain about services taking too long to shutdown. The other > > issue, > > > as Jay pointed out, is that the current behavior of close() has an > > > effectively small timeout, so this changes the behavior of existing > code, > > > which seems best to avoid. > > > > > > -Jason > > > > > > On Thu, Jan 5, 2017 at 3:09 PM, Gwen Shapira <g...@confluent.io> > wrote: > > > > > > > I hate going back and forth on this, but KafkaProducer.close() (with > > > > no timeout) is equivalent to close(Long.MAX_VALUE, > > > > TimeUnit.MILLISECONDS), while the KafkaConsumer.close() is equivalent > > > > to close(30*1000,TimeUnit.MILLISECONDS). > > > > > > > > Isn't this kind of inconsistency best to avoid? > > > > > > > > On Thu, Jan 5, 2017 at 2:08 PM, Rajini Sivaram < > > rajinisiva...@gmail.com> > > > > wrote: > > > > > Thank you, Ismael. I have sent another one. Hopefully that will > > appear > > > in > > > > > its own thread. > > > > > > > > > > Rajini > > > > > > > > > > On Thu, Jan 5, 2017 at 9:30 PM, Ismael Juma <ism...@juma.me.uk> > > wrote: > > > > > > > > > >> Thanks Rajini. This seems to be happening a lot lately: Gmail is > > > showing > > > > >> the vote message in the discuss thread. > > > > >> > > > > >> Ismael > > > > >> > > > > >> On Thu, Jan 5, 2017 at 9:23 PM, Rajini Sivaram < > > > rajinisiva...@gmail.com > > > > > > > > > >> wrote: > > > > >> > > > > >> > Hi all, > > > > >> > > > > > >> > I would like to start the voting process for *KIP-102 - Add > close > > > with > > > > >> > timeout for consumers:* > > > > >> > > > > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > >> > 102+-+Add+close+with+timeout+for+consumers > > > > >> > > > > > >> > > > > > >> > This KIP adds a new close method with a timeout for consumers > > > similar > > > > to > > > > >> > the close method in the producer. As described in the discussion > > > > thread > > > > >> > <http://mail-archives.apache.org/mod_mbox/kafka-dev/201612. > > > > mbox/%3cCAG_+ > > > > >> > n9us5ohthwmyai9pz4s2j62fmils2ufj8oie9jpmyaf...@mail.gmail.com > > %3e>, > > > > >> > the changes are only in the close code path and hence the impact > > is > > > > not > > > > >> too > > > > >> > big. The existing close() method without a timeout will use a > > > default > > > > >> > timeout of 30 seconds. > > > > >> > > > > > >> > Thank you... > > > > >> > > > > > >> > Regards, > > > > >> > > > > > >> > Rajini > > > > >> > > > > > >> > > > > > > > > > > > > > > > > -- > > > > Gwen Shapira > > > > Product Manager | Confluent > > > > 650.450.2760 | @gwenshap > > > > Follow us: Twitter | blog > > > > > > > > > >
