Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

> The unit of frequency is 1/s. What we are actually setting with this > config is the period. So frequency is used a bit wrongly here. Since I > discovered that we use the word "frequency" also for other similar > configs, I leave it up to you if you want to change it in this case or > not. I am f

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Nick, Dropping the `min.` prefix is fine with me. A really minor nit-picking: "The minimum frequency in milliseconds ..." The unit of frequency is 1/s. What we are actually setting with this config is the period. So frequency is used a bit wrongly here. Since I discovered that we use the

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Matthias, You raise a good point: commit.interval.ms only specifies a minimum interval, because commits are powered by record flow through the Topology, if there are no input records, there will be no commits. The parallel here is that repartition record purges are powered by commits, and if th

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Thanks for the KIP! Very good discussion. I want to raise one point: I am not a fan of the `min.` prefix of the config, because all configs like this are _minimums_. We also use `commit.interval.ms` and not `min.commit.interval.ms` for example. I would suggest to strip the `min.` prefix. Fo

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Thanks Bruno, I've made those changes. I'll call a vote on the KIP later today. Regards, Nick Telford On Wed, 12 Jan 2022 at 12:13, Bruno Cadonna wrote: > Hi Nick, > > Great! > > I think the KIP is ready for voting. I have just a couple of minor > comments. > > a. > In the config description,

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Nick, Great! I think the KIP is ready for voting. I have just a couple of minor comments. a. In the config description, I would replace "Purging will occur after at least this value since the last purge, but may be delayed until later in order to meet the processing guarantee." with "Pu

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Bruno, Thanks for your recommendations. I've made those changes, and also updated the description for the new config to read: "The minimum frequency in milliseconds with which to delete fully consumed records from repartition topics. *Purging will occur after at least this value since the last

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Nick, Sorry for the delay! Regarding point 7, I am fine with keeping the config as an interval and keeping it independently of the commit interval. However, I would then remove the following paragraph from the KIP: "We will still wait for a commit before explicitly deleting repartition r

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

> > You missed one "delete.interval.ms" in the last paragraph in section > "Proposed Changes". > I've fixed these now, including the title that I somehow missed! I am afraid, I again need to comment on point 7. IMO, it does not make sense to be able to tune repartition.purge.interval.ms and com

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Nick, Thanks for the updates! The motivation section and the description of the new config is much clearer and informative now! You missed one "delete.interval.ms" in the last paragraph in section "Proposed Changes". I am afraid, I again need to comment on point 7. IMO, it does not make

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi everyone, Thanks for your feedback. I've made the suggested changes to the KIP (1, 2, 3 and 5). For the new name, I've chosen repartition.purge.interval.ms, as I felt it struck a good balance between being self-descriptive and concise. Please let me know if you'd prefer something else. On poi

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Thanks, Bruno. I agree my point 4 is more like PR discussion, not KIP discussion. @Nick , please update my point 4 in PR directly. Thank you. Luke On Tue, Dec 21, 2021 at 7:24 PM Bruno Cadonna wrote: > Hi Nick, > > Thank you for the KIP! > > I agree on points 1, 2, and 3. I am not sure abou

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Nick, Thank you for the KIP! I agree on points 1, 2, and 3. I am not sure about point 4. I agree that we should update the docs for commit.interval.ms but I am not sure if this needs to mentioned in the KIP. That seems to me more a PR discussion. Also on point 2, I agree that we need to ad

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hi Nick, Thanks for the KIP! In addition to Sophie's comments, I have one more to this KIP: 3. I think you should mention the behavior change *explicitly* in "Compatibility" section. I know you already mentioned it in KIP, in the benefit way. But I think in this section, we should clearly point o

Re: [DISCUSS] KIP-811 Add separate delete.interval.ms to Kafka Streams

Hey Nick, I think you forgot to link to the KIP document, but I take it this is it: KIP-811: Add separate delete.interval.ms to Kafka Streams The overall proposal sounds good to me, just a few minor things: 1. Please specify everything needed to