Hi Jason, The initial (and still only) requirement I wanted out of this KIP was to have the header strategy. This is because I want to be able to version both by date/time or by (externally provided) sequence, this is specially important if you are running in multiple environments, which may cause the commonly known issue of the clocks being slightly de-synchronized. Having the timestamp strategy was added to the KIP as the result of the discussions, where it was seen as a potential benefit for other clients who may prefer that.
Cheers, Luís From: Jason Gustafson Sent: 10 August 2018 22:38 To: dev Subject: Re: [VOTE] KIP-280: Enhanced log compaction Hi Luis, It's still not very clear to me why we need the header-based strategy. Can you elaborate why having the timestamp-based approach alone is not sufficient? The use case in the motivation just describes a "most recent snapshot" use case. Thanks, Jason On Thu, Aug 9, 2018 at 4:36 AM, Luís Cabral <luis_cab...@yahoo.com.invalid> wrote: > Hi, > > > So, after a "short" break, I've finally managed to find time to resume > this KIP. Sorry to all for the delay. > > Continuing the conversation of the configurations being global vs topic, > I've checked this and it seems that they are only available globally. > > This configuration is passed to the log cleaner via "CleanerConfig.scala", > which only accepts global configurations. This seems intentional, as the > log cleaner is not mutable and doesn't get instantiated that often. I think > that changing this to accept per-topic configuration would be very nice, > but perhaps as a part of a different KIP. > > > Following the Kafka documentation, these are the settings I'm referring to: > > -- -- > > Updating Log Cleaner Configs > > Log cleaner configs may be updated dynamically at cluster-default > level used by all brokers. The changes take effect on the next iteration of > log cleaning. One or more of these configs may be updated: > > * log.cleaner.threads > > * log.cleaner.io.max.bytes.per.second > > * log.cleaner.dedupe.buffer.size > > * log.cleaner.io.buffer.size > > * log.cleaner.io.buffer.load.factor > > * log.cleaner.backoff.ms > > -- -- > > > > Please feel free to confirm, otherwise I will update the KIP to reflect > these configuration nuances in the next few days. > > > Best Regards, > > Luis > > > > On Monday, July 9, 2018, 1:57:38 PM GMT+2, Andras Beni < > andrasb...@cloudera.com.INVALID> wrote: > > > > > > Hi Luís, > > Can you please clarify how the header value has to be encoded in case log > compaction strategy is 'header'. As I see current PR reads varLong in > CleanerCache.extractVersion and read String and uses toLong in > Cleaner.extractVersion while the KIP says no more than 'the header value > (which must be of type "long")'. > > Otherwise +1 for the KIP > > As for current implementation: it seems in Cleaner class header key > "version" is hardwired. > > Andras > > > > On Fri, Jul 6, 2018 at 10:36 PM Jun Rao <j...@confluent.io> wrote: > > > 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 > > > > > >
