Hi, Guozhang, For #4, what you suggested could make sense for timestamp based de-dup, but not sure how general it is since the KIP also supports de-dup based on header.
Thanks, Jun On Fri, Jul 6, 2018 at 1:12 PM, Guozhang Wang <wangg...@gmail.com> wrote: > Hello Jun, > Thanks for your feedbacks. I'd agree on #3 that it's worth adding a special > check to not delete the last message, since although unlikely, it is still > possible that a new active segment gets rolled out but contains no data > yet, and hence the actual last message in this case would be in a > "compact-able" segment. > > For the second part of #4 you raised, maybe we could educate users to set " > message.timestamp.difference.max.ms" to be no larger than " > log.cleaner.delete.retention.ms" (its default value is Long.MAX_VALUE)? A > more aggressive approach would be changing the default value of the former > to be the value of the latter if: > > 1. cleanup.policy = compact OR compact,delete > 2. log.cleaner.compaction.strategy != offset > > Because in this case I think it makes sense to really allow users send any > data longer than "log.cleaner.delete.retention.ms", WDYT? > > > Guozhang > > > On Fri, Jul 6, 2018 at 11:51 AM, Jun Rao <j...@confluent.io> wrote: > > > Hi, Luis, > > > > 1. The cleaning policy is configurable at both global and topic level. > The > > global one has the name log.cleanup.policy and the topic level has the > name > > cleanup.policy by just stripping the log prefix. We can probably do the > > same for the new configs. > > > > 2. Since this KIP may require an admin to configure a larger dedup buffer > > size, it would be useful to document this impact in the wiki and the > > release notes. > > > > 3. Yes, it's unlikely for the last message to be removed in the current > > implementation since we never clean the active segment. However, in > theory, > > this can happen. So it would be useful to guard this explicitly. > > > > 4. Just thought about another issue. We probably want to be a bit careful > > with key deletion. Currently, one can delete a key by sending a message > > with a delete tombstone (a null payload). To prevent a reader from > missing > > a deletion if it's removed too quickly, we depend on a configuration > > log.cleaner.delete.retention.ms (defaults to 1 day). The delete > tombstone > > will only be physically removed from the log after that amount of time. > The > > expectation is that a reader should finish reading to the end of the log > > after consuming a message within that configured time. With the new > > strategy, we have similar, but slightly different problems. The first > > problem is that the delete tombstone may be delivered earlier than an > > outdated record in offset order to a consumer. In order for the consumer > > not to take the outdated record, the consumer should cache the deletion > > tombstone for some configured amount of time. We ca probably piggyback > this > > on log.cleaner.delete.retention.ms, but we need to document this. The > > second problem is that once the delete tombstone is physically removed > from > > the log, how can we prevent outdated records to be added (otherwise, they > > will never be garbage collected)? Not sure what's the best way to do > this. > > One possible way is to push this back to the application and require the > > user not to publish outdated records after log.cleaner.delete.retention. > ms > > . > > > > Thanks, > > > > Jun > > > > On Wed, Jul 4, 2018 at 11:11 AM, Luís Cabral > <luis_cab...@yahoo.com.invalid > > > > > wrote: > > > > > Hi Jun, > > > > > > -: 1. I guess both new configurations will be at the topic level? > > > > > > They will exist in the global configuration, at the very least. > > > I would like to have them on the topic level as well, but there is an > > > inconsistency between the cleanup/compaction properties that exist > “only > > > globally” vs “globally + per topic”. > > > I haven’t gotten around to investigating why, and if that reason would > > > then also impact the properties I’m suggesting. At first glance they > seem > > > to belong with the properties that are "only globally” configured, but > > > Guozhang has written earlier with a suggestion of a compaction property > > > that works for both (though I haven’t had the time to look into it yet, > > > unfortunately). > > > > > > -: 2. Since the log cleaner now needs to keep both the offset and > > another > > > long (say timestamp) in the de-dup map, it reduces the number of keys > > that > > > we can keep in the map and thus may require more rounds of cleaning. > This > > > is probably not a big issue, but it will be useful to document this > > impact > > > in the KIP. > > > > > > As a reader, I tend to prefer brief documentation on new features (they > > > tend to be too many for me to find the willpower to read a 200-page > essay > > > about each one), so that influences me to avoid writing about every > > > micro-impact that may exist, and simply leave it inferred (as you have > > just > > > done). > > > But I also don’t feel strongly enough about it to argue either way. So, > > > after reading my argument, if you still insist, I’ll happily add this > > there. > > > > > > -: 3. With the new cleaning strategy, we want to be a bit careful with > > > removing the last message in a partition (which is possible now). We > need > > > to preserve the offset of the last message so that we don't reuse the > > > offset for a different message. One way to simply never remove the last > > > message. Another way (suggested by Jason) is to create an empty message > > > batch. > > > > > > That is a good point, but isn’t the last message always kept > regardless? > > > In all of my tests with this approach, never have I seen it being > > removed. > > > This is not because I made it so while changing the code, it was simply > > > like this before, which made me smile! > > > Given these results, I just *assumed* (oops) that these scenarios > > > represented the reality, so the compaction would only affected the > > “tail”, > > > while the “head” remained untouched. Now that you say its possible that > > the > > > last message actually gets overwritten somehow, I guess a new bullet > > point > > > will have to be added to the KIP for this (after I’ve found the time to > > > review the portion of the code that enacts this behaviour). > > > > > > Kind Regards, > > > Luís Cabral > > > > > > From: Jun Rao > > > Sent: 03 July 2018 23:58 > > > To: dev > > > Subject: Re: [VOTE] KIP-280: Enhanced log compaction > > > > > > Hi, Luis, > > > > > > Thanks for the KIP. Overall, this seems a useful KIP. A few comments > > below. > > > > > > 1. I guess both new configurations will be at the topic level? > > > 2. Since the log cleaner now needs to keep both the offset and another > > long > > > (say timestamp) in the de-dup map, it reduces the number of keys that > we > > > can keep in the map and thus may require more rounds of cleaning. This > is > > > probably not a big issue, but it will be useful to document this impact > > in > > > the KIP. > > > 3. With the new cleaning strategy, we want to be a bit careful with > > > removing the last message in a partition (which is possible now). We > need > > > to preserve the offset of the last message so that we don't reuse the > > > offset for a different message. One way to simply never remove the last > > > message. Another way (suggested by Jason) is to create an empty message > > > batch. > > > > > > Jun > > > > > > On Sat, Jun 9, 2018 at 12:39 AM, Luís Cabral > > <luis_cab...@yahoo.com.invalid > > > > > > > wrote: > > > > > > > Hi all, > > > > > > > > Any takers on having a look at this KIP and voting on it? > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > 280%3A+Enhanced+log+compaction > > > > > > > > Cheers, > > > > Luis > > > > > > > > > > > > > > > > -- > -- Guozhang >
