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 <cado...@apache.org> wrote: > 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 add a doc string > but the content should be exemplary not binding. What I want to say is > that, we do not need a KIP to change docs. > > Here my points: > > 5. Could you specify in the motivation that the KIP is about deleting > records from repartition topics? Maybe with a short description when why > and when records are deleted from the repartition topics. For us it > might be clear, but IMO we should try to write KIPs so that someone that > is relatively new to Kafka Streams can understand the KIP without > needing to know too much background. > > 6. Does the config need to be validated? For example, does > delete.interval.ms need to be greater than or equal to commit.interval.ms? > > 7. Should the default value for non-EOS be 30s or the same value as > commit.interval.ms? I am just thinking about the case where a user > explicitly changes commit.interval.ms but not delete.interval.ms (or > whatever name you come up for it). Once delete.interval.ms is set > explicitly it is decoupled from commit.interval.ms. Similar could be > done for the EOS case. > Alternatively, we could also define delete.interval.ms to take a > integral number without a unit that specifies after how many commit > intervals the records in repartition topics should be deleted. This > would make sense since delete.interval.ms is tightly bound to > commit.interval.ms. Additionally, it would make the semantics of the > config simpler. The name of the config should definitely change if we go > down this way. > > Best, > Bruno > > > > On 21.12.21 11:14, Luke Chen wrote: > > 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 > >>> > >> > > >
