I agree with Ismael that we should remove -1. I think we tend to view the coupling of these behaviors into a single configuration as a mistake, so it's a little odd to keep it (even if in a weakened form).
-Jason On Sat, Jul 30, 2022 at 7:37 AM Ismael Juma <ism...@juma.me.uk> wrote: > I would remove -1 altogether. Two more comments: > > 1. The current description kind of leads people towards aligning the config > with delivery.timeout.ms. Is that what we want? We could say it should be > higher than delivery.timeout.ms but indicate that the default is usually > fine. The main reason to reduce it would be to save memory, I guess. > 2. Each config has a priority, we should specify it for this one. I'm > assuming it will be "low". > > Ismael > > On Fri, Jul 29, 2022 at 2:38 PM Justine Olshan > <jols...@confluent.io.invalid> > wrote: > > > Hi all, > > > > I've updated the KIP to include the new default of 1 day and information > > about -1 in the description of the config. > > I wonder though if including -1 makes sense now that it is not the > default > > value. Is there a benefit for manually setting -1 vs manually setting the > > value that transactional.id.expiration.ms has? > > > > Let me know your thoughts. > > Thanks, > > Justine > > > > On Fri, Jul 29, 2022 at 10:54 AM Ismael Juma <ism...@juma.me.uk> wrote: > > > > > +1 for having 1 day as the default and for including this change in the > > > release notes. > > > > > > Ismael > > > > > > On Wed, Jul 27, 2022 at 9:16 AM Jason Gustafson > > <ja...@confluent.io.invalid > > > > > > > wrote: > > > > > > > I don't think a major release is a requirement for a default change > for > > > > what it's worth. I do appreciate that there is a preference for not > > > rocking > > > > the boat though. For a little bit of background here, the problem we > > > > have encountered in production since the idempotent producer became > the > > > > default is OOM errors due to huge numbers of producerIds that had to > be > > > > retained in the cache for 7 days. It is hard to prevent use cases > from > > > > emerging where producers are used and discarded rapidly. We will be > > > using a > > > > lower value for sure, but it would also be nice to reduce the > > likelihood > > > > for the community to see this problem. The benefit of the caching > > > > diminishes quickly over time since it is primarily meant to handle > > > producer > > > > retry windows. I do not think there is much difference between 1 days > > > and 7 > > > > days from an application perspective, but it is a huge difference for > > the > > > > broker's memory usage. > > > > > > > > Best, > > > > Jason > > > > > > > > On Wed, Jul 27, 2022 at 2:57 AM Sagar <sagarmeansoc...@gmail.com> > > wrote: > > > > > > > > > Thanks Justine for the KIP. I think it might be better to document > > the > > > > > correlation between the new config and delivery.timeout.ms in the > > > Public > > > > > Interfaces Description. > > > > > > > > > > Also, I agree with Luke that for now setting a default to -1 should > > be > > > > > good. We can look to switch to 1 day with major release. > > > > > > > > > > Thanks! > > > > > Sagar. > > > > > > > > > > On Wed, Jul 27, 2022 at 9:05 AM Luke Chen <show...@gmail.com> > wrote: > > > > > > > > > > > Hi Justine, > > > > > > > > > > > > Thanks for the KIP. > > > > > > I agree with you that we should try our best to keep backward > > > > > > compatibility, although our intention is to have lower producer > id > > > > > > expiration timeout. > > > > > > So, I think we should keep default to -1 IMO. > > > > > > Maybe we change the default to 1 day in next major release (4.0)? > > > > > > > > > > > > Thank you. > > > > > > Luke > > > > > > > > > > > > On Wed, Jul 27, 2022 at 7:13 AM Justine Olshan > > > > > > <jols...@confluent.io.invalid> > > > > > > wrote: > > > > > > > > > > > > > Thanks for taking a look Jason! > > > > > > > > > > > > > > I wondered if we wanted to have a smaller default but wasn't > sure > > > > about > > > > > > the > > > > > > > compatibility story -- especially since there is the chance for > > > > > producer > > > > > > > IDs to expire silently. > > > > > > > I do think that 1 day is fairly reasonable. If I don't hear any > > > > > > conflicting > > > > > > > opinions I can go ahead and update the default. > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > On Tue, Jul 26, 2022 at 12:23 PM Jason Gustafson > > > > > > > <ja...@confluent.io.invalid> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Justine, > > > > > > > > > > > > > > > > Thanks for the KIP. Although I hate seeing new > configurations, > > I > > > > > think > > > > > > > this > > > > > > > > is a good change. Combining these timeout behaviors into a > > single > > > > > > > > configuration was definitely a mistake, but we didn't > > anticipate > > > > the > > > > > > > > problem with the producer id cache. I do wonder if we can > make > > > the > > > > > > > default > > > > > > > > a bit lower to reduce the chances that users will hit the > same > > > > memory > > > > > > > > issues we have seen. After decoupling this configuration from > > > > > > > > transactional.id.expiration.ms, the new timeout just needs > to > > > > cover > > > > > > the > > > > > > > > longest duration that a producer might be retrying the same > > > Produce > > > > > > > > request. 7 days seems too high. Although I think it could go > a > > > fair > > > > > > even > > > > > > > > lower, perhaps 1 day is a reasonable place to start? > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Jason > > > > > > > > > > > > > > > > On Mon, Jul 25, 2022 at 9:25 AM Justine Olshan > > > > > > > > <jols...@confluent.io.invalid> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hey Bill, > > > > > > > > > Thanks! I was just going to say that hopefully > > > > > > > > > transactional.id.expiration.ms would also be over the > > delivery > > > > > > > timeout. > > > > > > > > :) > > > > > > > > > Thanks for the +1! > > > > > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > > > > > On Mon, Jul 25, 2022 at 9:17 AM Bill Bejeck < > > bbej...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Justine, > > > > > > > > > > > > > > > > > > > > I just took another look at the KIP, and I realize my > > > > > > > > question/suggestion > > > > > > > > > > about default values has already been addressed in the > > > > > > > `Compatibility` > > > > > > > > > > section. > > > > > > > > > > > > > > > > > > > > I'm +1 on the KIP. > > > > > > > > > > > > > > > > > > > > -Bill > > > > > > > > > > > > > > > > > > > > On Thu, Jul 21, 2022 at 6:20 PM Bill Bejeck < > > > bbej...@gmail.com > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Justine, > > > > > > > > > > > > > > > > > > > > > > Thanks for the well written KIP, this looks like it > will > > > be a > > > > > > > useful > > > > > > > > > > > addition. > > > > > > > > > > > > > > > > > > > > > > Overall the KIP looks good to me, I have one > > > > question/comment. > > > > > > > > > > > > > > > > > > > > > > You mentioned that setting the ` > > producer.id.expiration.ms` > > > > > less > > > > > > > than > > > > > > > > > the > > > > > > > > > > > delivery timeout could lead to duplicates, which makes > > > sense. > > > > > To > > > > > > > > help > > > > > > > > > > > avoid this situation, do we want to consider a default > > > value > > > > > that > > > > > > > is > > > > > > > > > the > > > > > > > > > > > same as the delivery timeout? > > > > > > > > > > > > > > > > > > > > > > Thanks again for the KIP. > > > > > > > > > > > > > > > > > > > > > > Bill > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 21, 2022 at 4:54 PM Justine Olshan > > > > > > > > > > > <jols...@confluent.io.invalid> wrote: > > > > > > > > > > > > > > > > > > > > > >> Hey all! > > > > > > > > > > >> > > > > > > > > > > >> I'd like to start a discussion on my proposal to > > separate > > > > > > > time-based > > > > > > > > > > >> producer ID expiration from transactional ID > expiration > > by > > > > > > > > > introducing a > > > > > > > > > > >> new configuration. > > > > > > > > > > >> > > > > > > > > > > >> The KIP Is pretty small and simple, but will be > helpful > > in > > > > > > > > controlling > > > > > > > > > > >> memory usage in brokers -- especially now that by > > default > > > > > > > producers > > > > > > > > > are > > > > > > > > > > >> idempotent and create producer ID state. > > > > > > > > > > >> > > > > > > > > > > >> Please take a look and leave any comments you may > have! > > > > > > > > > > >> > > > > > > > > > > >> KIP: > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-854+Separate+configuration+for+producer+ID+expiry > > > > > > > > > > >> JIRA: > https://issues.apache.org/jira/browse/KAFKA-14097 > > > > > > > > > > >> > > > > > > > > > > >> Thanks! > > > > > > > > > > >> Justine > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
