To make it clear, I don't against using -1 as disabled, but we need to come up with the meaning of "0". If "0" means immediate compaction, but the actual compaction lag will be segment.ms. It has longer lag than setting the value to be half of segment.ms. We cannot provide "0" as max compaction lag.
Here are two options. Option 1: Keep 0 as disabled Option 2: -1 (disabled), 0 (max compaction lag = segment.ms), and others. Xiongqi (Wesley) Wu On Wed, Sep 5, 2018 at 5:49 PM Brett Rann <br...@zendesk.com.invalid> wrote: > -1 is consistent as "special" with these settings for example: > > log.retention.bytes > socket.received.buffer.bytes > socket.send.buffer.bytes > queued.max.request.bytes > retention.bytes > retention.ms > > and acks. > > Where it may mean no limit, use OS defaults, max (acks), etc. I don't see > much convention of 0 meaning those things. > > There are some NULLs but it seems convetion there is NULL is used where > there's another setting in the hierarchy that would be used instead. > > > > > On Thu, Sep 6, 2018 at 10:42 AM Brett Rann <br...@zendesk.com> wrote: > > > If segment.ms can't be set to 0, then we're not being consistent > > by using 0 for this new setting? I throw out -1 for consideration > > again :) > > > > On Thu, Sep 6, 2018 at 10:03 AM xiongqi wu <xiongq...@gmail.com> wrote: > > > >> Thanks. I will document after PR is merged. > >> > >> BTW, Kafka enforce the minimum of "segment.ms" to 1, we cannot set " > >> segment.ms" to 0. > >> > >> I also updated the title of this KIP. > >> > >> Xiongqi (Wesley) Wu > >> > >> > >> On Wed, Sep 5, 2018 at 4:34 PM Brett Rann <br...@zendesk.com.invalid> > >> wrote: > >> > >> > I withdraw my comments on -1 since i'm in the minority. :) Can we > >> > make sure 0 gets documented as meaning disabled here: > >> > https://kafka.apache.org/documentation/#brokerconfigs > >> <https://kafka.apache.org/documentation/#brokerconfigs> ? > >> > And while there it would be good if segment.ms is documented > >> > that 0 is disabled too. (there's some hierarchy of configs for that > too > >> > if its not set and null for others means disabled!) > >> > > >> > > >> > On Thu, Sep 6, 2018 at 4:44 AM xiongqi wu <xiongq...@gmail.com> > wrote: > >> > > >> > > If we use 0 to indicate immediate compaction, the compaction lag is > >> > > determined by segment.ms in worst case. If segment.ms is 24 hours, > >> > > "immediate compaction" is a weaker guarantee than setting any value > >> less > >> > > than 24 hours. By the definition of "max compaction lag", we cannot > >> have > >> > > zero lag. So I use 0 to indicate "disable". > >> > > > >> > > > >> > > > >> > > Xiongqi (Wesley) Wu > >> > > > >> > > > >> > > On Wed, Sep 5, 2018 at 8:34 AM Colin McCabe <cmcc...@apache.org> > >> wrote: > >> > > > >> > > > On Tue, Sep 4, 2018, at 22:11, Brett Rann wrote: > >> > > > > > That's a fair point. We should make 0 = disable, to be > >> consistent > >> > > with > >> > > > > the other settings. > >> > > > > > >> > > > > -1 is used elsewhere for disable and when seeing it in a config > >> it's > >> > > > clear > >> > > > > that it's a special meaning. 0 doesn't have to mean instant, it > >> just > >> > > > means > >> > > > > as quickly as possible. I don't think 0 is intuitive for > disabled > >> and > >> > > it > >> > > > > will be confusing. I wasn't aware segment.ms=0 == disabled, > but I > >> > > think > >> > > > > that is also unintuitive. > >> > > > > >> > > > I think there is an argument for keeping these two configurations > >> > > > consistent, since they are so similar. I agree that 0 was an > >> > unfortunate > >> > > > choice., > >> > > > > >> > > > best, > >> > > > Colin > >> > > > > >> > > > > > >> > > > > On Wed, Sep 5, 2018 at 11:38 AM Colin McCabe < > cmcc...@apache.org> > >> > > wrote: > >> > > > > > >> > > > > > On Tue, Sep 4, 2018, at 17:47, xiongqi wu wrote: > >> > > > > > > Colin, > >> > > > > > > Thank you for comments. > >> > > > > > > see my inline reply below. > >> > > > > > > > >> > > > > > > Xiongqi (Wesley) Wu > >> > > > > > > > >> > > > > > > > >> > > > > > > On Tue, Sep 4, 2018 at 5:24 PM Colin McCabe < > >> cmcc...@apache.org> > >> > > > wrote: > >> > > > > > > > >> > > > > > > > Hi Xiongqi, > >> > > > > > > > > >> > > > > > > > Thanks for this KIP. > >> > > > > > > > > >> > > > > > > > The name seems a bit ambiguous. Our compaction policies > are > >> > > already > >> > > > > > > > time-based, after all. It seems like this change is > focused > >> > > around > >> > > > > > adding > >> > > > > > > > a “max.compaction.lag.ms." Perhaps the KIP title should > be > >> > > > something > >> > > > > > > > like "add maximum compaction lag time"? > >> > > > > > > > > >> > > > > > > > ==========> sure. I will change the title. > >> > > > > > > > >> > > > > > > > The active segment is forced to roll when either " > >> > > > > > max.compaction.lag.ms" > >> > > > > > > > > or "segment.ms" (log.roll.ms and log.roll.hours) has > >> > reached. > >> > > > > > > > > >> > > > > > > > If the max.compaction.lag.ms is low, it seems like > segments > >> > will > >> > > > be > >> > > > > > > > rolled very frequently. This can be a source of problems > in > >> the > >> > > > > > cluster, > >> > > > > > > > since creating many different small log segments consumes > a > >> > huge > >> > > > > > amount of > >> > > > > > > > cluster resources. Therefore, I would suggest adding a > >> > > broker-level > >> > > > > > > > configuration which allows us to set a minimum value for > >> > > > > > > > max.compaction.lag.ms. If we let users set it on a > >> per-topic > >> > > > basis, > >> > > > > > > > someone could set a value of 1 ms or something, and cause > >> > chaos. > >> > > > > > > > > >> > > > > > > > =========> this applies to segment.ms as well. Today > users > >> can > >> > > > set " > >> > > > > > > segment.ms" to a very low value, and cause a frequent > >> rolling of > >> > > > active > >> > > > > > > segments. > >> > > > > > > >> > > > > > Hi Xiongqi, > >> > > > > > > >> > > > > > I agree that this is an existing problem with segment.ms. > >> However, > >> > > > that > >> > > > > > doesn't mean that we shouldn't fix it. As you noted, there > will > >> be > >> > > more > >> > > > > > interest in these topic-level retention settings as a result > of > >> > GDPR. > >> > > > It > >> > > > > > seems likely that pre-existing problems will cause more > trouble. > >> > > > > > > >> > > > > > The fix seems relatively straightforward here -- add a > >> broker-level > >> > > > > > minimum segment.ms that overrides per-topic minimums. We can > >> also > >> > > fail > >> > > > > > with a helpful error message when someone attempts to set an > >> > invalid > >> > > > > > configuration. > >> > > > > > > >> > > > > > > In my option, the minimum of "max.compaction.lag.ms" should > >> be > >> > > > > > > based on the minimum of "segment.ms". Since today the > >> minimum of > >> > > > > > segment.ms > >> > > > > > > is 1, "max.compaction.lag.ms" also starts with 1. "0" means > >> > > > disable. I > >> > > > > > > can use -1 as disable, but it is hard to define the meaning > >> of 0 > >> > > > because > >> > > > > > we > >> > > > > > > cannot just roll the active segment immediately. > >> > > > > > > >> > > > > > That's a fair point. We should make 0 = disable, to be > >> consistent > >> > > with > >> > > > the > >> > > > > > other settings. > >> > > > > > > >> > > > > > best, > >> > > > > > Colin > >> > > > > > > >> > > > > > > > >> > > > > > > > -- Note that an alternative configuration is to use -1 as > >> > > > "disabled" > >> > > > > > and > >> > > > > > > > 0 > >> > > > > > > > > as "immediate compaction". Because compaction lag is > still > >> > > > determined > >> > > > > > > > > based on min.compaction.lag and how long to roll an > active > >> > > > segment, > >> > > > > > > > the > >> > > > > > > > > actual lag for compaction is undetermined if we use "0". > >> On > >> > the > >> > > > other > >> > > > > > > > > hand, we can already set "min.cleanable.dirty.ratio" to > >> > achieve > >> > > > the > >> > > > > > > > same > >> > > > > > > > > goal. So here we choose "0" as "disabled". > >> > > > > > > > > >> > > > > > > > I would prefer -1 to be the invalid setting. Treating 0 > >> > > differently > >> > > > > > than > >> > > > > > > > 1 seems strange to me. > >> > > > > > > > > >> > > > > > > > =====> see my previous comment, I am not strongly against, > >> but > >> > 0 > >> > > is > >> > > > > > not a > >> > > > > > > valid configuration in my option. So I use "0" as disabled > >> state. > >> > > > > > > > >> > > > > > > best, > >> > > > > > > > Colin > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > On Tue, Sep 4, 2018, at 15:04, xiongqi wu wrote: > >> > > > > > > > > Let's VOTE for this KIP. > >> > > > > > > > > KIP: > >> > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-354 > >> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354> > >> > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354 > >> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354>> > >> > > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354 > >> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354> > >> > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354 > >> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-354>>> > >> > > > > > > > > %3A+Time-based+log+compaction+policy > >> > > > > > > > > > >> > > > > > > > > Implementation: > >> > > > > > > > > > >> > > > > > > > > https://github.com/apache/kafka/pull/5611 > >> <https://github.com/apache/kafka/pull/5611> > >> > > <https://github.com/apache/kafka/pull/5611 > >> <https://github.com/apache/kafka/pull/5611>> > >> > > > > > <https://github.com/apache/kafka/pull/5611 > >> <https://github.com/apache/kafka/pull/5611> > >> > > <https://github.com/apache/kafka/pull/5611 > >> <https://github.com/apache/kafka/pull/5611>>> > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > Xiongqi (Wesley) Wu > >> > > > > > > > > >> > > > > > > >> > > > > >> > > > >> > > >> > > >> > -- > >> > > >> > Brett Rann > >> > > >> > Senior DevOps Engineer > >> > > >> > > >> > Zendesk International Ltd > >> > > >> > 395 Collins Street, Melbourne VIC 3000 Australia > >> > > >> > Mobile: +61 (0) 418 826 017 > >> > > >> > > > > > > -- > > > > Brett Rann > > > > Senior DevOps Engineer > > > > > > Zendesk International Ltd > > > > 395 Collins Street, Melbourne VIC 3000 Australia > > > > Mobile: +61 (0) 418 826 017 > > > > > -- > > Brett Rann > > Senior DevOps Engineer > > > Zendesk International Ltd > > 395 Collins Street, Melbourne VIC 3000 Australia > > Mobile: +61 (0) 418 826 017 >
