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 out which behavior will be change after this KIP. That is, you should put it clear that the delete record interval will change from 100ms to 30s with EOS enabled. And it should also be mentioned in doc/upgrade.html doc. 4. Since this new config has some relationship with commit.interval.ms, I think we should also update the doc description for `commit.interval.ms`, to let user know there's another config to control delete interval and should be greater than commit.interval.ms. Something like that. WDYT? (You should put this change in the KIP as Sophie mentioned) Thank you. Luke On Tue, Dec 21, 2021 at 9:27 AM Sophie Blee-Goldman <sop...@confluent.io.invalid> wrote: > 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 > <https://cwiki.apache.org/confluence/x/JY-kCw> > > The overall proposal sounds good to me, just a few minor things: > > 1. Please specify everything needed to define this config explicitly, ie > all the arguments that will be passed in to the > StreamsConfig's ConfigDef: in addition to the default value, we need the > config type (presumably a Long), the doc > string, and the importance (probably "low", similar to > commit.interval.ms > ) > 2. Might be worth considering a slightly more descriptive name for this > config. Most users probably don't think about, > or may not even be aware of, the deletion of consumed records by Kafka > Streams, so calling it something along > the lines of "repartition.records.delete.interval.ms" or " > consumed.records.deletion.interval.ms" or so on will help > make it clear what the config refers to and whether or not they need to > care. Maybe you can come up with better > and/or shorter names, just wanted to suggest some example names that I > think sufficiently get the point across > > Other than that I'm +1 -- thanks for the KIP! > > Sophie > > > > On Mon, Dec 20, 2021 at 9:15 AM Nick Telford <nick.telf...@gmail.com> > wrote: > > > This is a KIP for a proposed solution to KAFKA-13549 > > <https://issues.apache.org/jira/browse/KAFKA-13549>. The solution is > very > > simple, so the KIP is pretty short. > > > > The suggested changes are implemented by this PR > > <https://github.com/apache/kafka/pull/11610>. > > -- > > Nick Telford > > >
