+1 for internal configuration to reduce the complexity.
We can externalize it if we have some requirements in the future.
Thanks.
Luke
On Sat, Aug 20, 2022 at 2:13 AM David Jacot wrote:
> Sounds good. Thanks, Justine.
>
> Le ven. 19 août 2022 à 19:38, Justine Olshan >
> a écrit :
>
> > Hi all,
Sounds good. Thanks, Justine.
Le ven. 19 août 2022 à 19:38, Justine Olshan
a écrit :
> Hi all,
>
> Followed up with David and Ismael offline.
> Ismael explained that we probably don't want to increase complexity and
> didn't think the value needed to be modified beyond tests. I agree with
> this
Hi all,
Followed up with David and Ismael offline.
Ismael explained that we probably don't want to increase complexity and
didn't think the value needed to be modified beyond tests. I agree with
this so I've decided to go with an internal configuration (with the same
default as mentioned above).
Hi David,
I chose the name to match the variable name of the existing hardcoded
value. I also thought it was clearer what was happening.
I'm not sure how to follow the pattern above in a way that is not overly
verbose. It would have to be something like
producer.id.remove.expired.producer.id.clea
Given that we already have
`transaction.abort.timed.out.transaction.cleanup.interval.ms` and
`transaction.remove.expired.transaction.cleanup.interval.ms`, it seems
OK to add another one for our case here. Regarding the name, I would
follow the pattern that we use for those two existing configs. We
Hi Ismael,
We can do this if we think that an external configuration is not necessary.
Just wondering, is there a reason we don't want an external configuration
here?
Thanks,
Justine
On Wed, Aug 17, 2022 at 12:30 PM Ismael Juma wrote:
> Why does this have to be an external config? We can provi
Why does this have to be an external config? We can provide an internal
mechanism to configure this, no?
Ismael
On Wed, Aug 17, 2022 at 9:22 AM Justine Olshan
wrote:
> Hey all,
> Quick update to this KIP. While working on the PR and tests I realized that
> we have a hardcoded value for how ofte
Hey all,
Quick update to this KIP. While working on the PR and tests I realized that
we have a hardcoded value for how often we clean up producer IDs. Currently
this value is 10 minutes and is defined in LogManager.
I thought for better testing and ease of use of the new configuration, we
should al
Awesome. Thanks Tom!
I plan to open this KIP vote at the start of next week.
Thanks all for the discussion! Let me know if there is anything else. :)
Justine
On Wed, Aug 3, 2022 at 11:32 AM Tom Bentley wrote:
> Hi Justine,
>
> That all seems reasonable to me, thanks!
>
> On Wed, 3 Aug 2022 at
Hi Justine,
That all seems reasonable to me, thanks!
On Wed, 3 Aug 2022 at 19:14, Justine Olshan
wrote:
> Hi Tom and Ismael,
>
> 1. Yes, there are definitely many ways to improve this issue and I plan to
> write followup KIPs to address some of the larger changes.
> Just wanted to get this simp
Hi Tom and Ismael,
1. Yes, there are definitely many ways to improve this issue and I plan to
write followup KIPs to address some of the larger changes.
Just wanted to get this simple fix in as a short term measure to prevent
issues with too many producer IDs in the cache. Stay tuned :)
2. I did
Regarding 1, more can certainly be done, but I think it would be
complementary. As such, I think this KIP stands on its own and additional
improvements can be handled via future KIPs (unless Justine wants to
combine things, of course).
Ismael
On Wed, Aug 3, 2022 at 9:12 AM Tom Bentley wrote:
>
Hi Justine,
Thanks for the KIP! I can see that this is a pragmatic attempt to address a
nasty problem. I have a few questions:
1. The KIP makes the problem significantly harder to trigger, but doesn't
eliminate it entirely. How confident are you that it will be sufficient in
practice? We can poin
I've updated the KIP to make the new minimum value 1 and remove the -1
configuration.
I've also added the low priority to the configuration and edited the
description as Ismael mentioned.
I'm thinking about bringing this KIP to a vote soon! Let me know if there
are any other comments or questions.
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 wrote:
> I would remove -1 altogether.
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 r
Hey Justine,
Thanks for that. In a way you are right, that setting the new config to -1
is equivalent to setting it equal to transactional.id.expiration.ms . The
only reason I mentioned the -1 case is it gives the users the ability to
disable this feature (which mayn't even be desirable based on w
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.
+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
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 ro
Thanks Justine,
I thought -1 might be a good default setting for backward compatibility
reasons. I am not too adamant on it either. Probably worth mentioning in
the description that setting it to -1 would disable the feature?
Other than that, LGTM!
Thanks!
Sagar.
On Fri, Jul 29, 2022 at 8:37 AM
Hi Jason,
Thanks for the info. I don't strongly insist on making the default to -1
for backward compatibility.
If other people in the community also agree with the change, I'm good with
that.
Thank you.
Luke
On Fri, Jul 29, 2022 at 5:35 AM Justine Olshan
wrote:
> Thanks Jason, Luke, Sagar,
Thanks Jason, Luke, Sagar, and Kirk,
Seems like there is still some debate over the default value. I think there
is a general consensus that we can reduce the default at some point, but
exactly when is still not clear. I do think Jason made a good point about
applications taking 1 day to retry. I
Hi Justine,
Thanks for the KIP. I appreciated the background context and clarity you added.
On Wed, Jul 27, 2022, at 2:57 AM, Sagar 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
> Interfac
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
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.
T
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)?
Than
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
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
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 wrote:
> Hi Justine,
>
> I just took another look at the KIP, and I realize my question/sugg
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 wrote:
> Hi Justine,
>
> Thanks for the well written KIP,
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 hel
31 matches
Mail list logo
