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 > > >
