OK thanks for that clarification. I see why you're uncomfortable with 0 now.
I'm not really fussed. I just prefer consistency in configuration options. Personally I lean towards treating 0 and 1 similarly in that scenario, because it favours the person thinking about setting the configurations, and a person doesn't care about a 1ms edge case especially when the context is the true minimum is tied to the log cleaner cadence. Introducing 0 to mean "disabled" because there is some uniquness in segment.ms not being able to be set to 0, reduces configuration consistency in favour of capturing a MS gap in an edge case that nobody would ever notice. For someone to understand why everywhere else -1 is used to disable, but here 0 is used, they would need to learn about segment.ms having a 1ms minimum and then after learning would think "who cares about 1ms?" in this context. I would anyway :) my 2c anyway. Will again defer to majority. Curious which way Colin falls now. Don't want to spend more time on this though, It's well into bikeshedding territory now. :) On Thu, Sep 6, 2018 at 1:31 PM xiongqi wu <xiongq...@gmail.com> wrote: > I want to honor the minimum value of segment.ms (which is 1ms) to force > roll an active segment. > So if we set "max.compaction.lag.ms" any value > 0, the minimum of > max.compaction.lag.ms and segment.ms will be used to seal an active > segment. > > If we set max.compaction.lag.ms to 0, the current implementation will > treat it as disabled. > > It is a little bit weird to treat max.compaction.lag=0 the same as > max.compaction.lag=1. > > There might be a reason why we set the minimum of segment.ms to 1, and I > don't want to break this assumption. > > > > Xiongqi (Wesley) Wu > > > On Wed, Sep 5, 2018 at 7:54 PM Brett Rann <br...@zendesk.com.invalid> > wrote: > > > You're rolling a new segment if the condition is met right? So I'm > > struggling to understand the relevance of segment.ms here. Maybe an > > example > > would help my understanding: > > > > segment.ms=9999999 > > *min.cleanable.dirty.ratio=1* > > max.compaction.lag.ms=1 > > > > When a duplicate message comes in, after 1ms the topic should be eligible > > for compaction when the log compaction thread gets around to evaluating > the > > topic. > > > > if we have > > segment.ms=9999999 > > *min.cleanable.dirty.ratio=1* > > max.compaction.lag.ms=0 > > > > When a duplicate message comes in, after 0ms the topic should be eligible > > for compaction when the log compaction thread gets around to evaluating > the > > topic. > > > > In both of those cases the change would mean a new segment is rolled so > the > > new message would be part of the compaction task. 0 and 1 are practically > > the same meaning since neither is providing an actual guarantee at such > low > > MS settings, but effectively tying it to both the frequency of the log > > cleaner running and the priority of the given topic being the highest > > priority of all topics that are evaluated for cleaning on the next cycle. > > You've captured that nuance with careful "skipped" wording in the KIP > > here "controls > > the max time interval a message/segment can be skipped for log > compaction". > > > > How is 0 different to 1, practically? And how is it relating to > segment.ms > > ? > > Is it that you're proposing to have 0 mean "use segment.ms instead?" as > a > > kind of third option? > > > > > > > > On Thu, Sep 6, 2018 at 11:34 AM xiongqi wu <xiongq...@gmail.com> wrote: > > > > > 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> > > > <https://kafka.apache.org/documentation/#brokerconfigs > <https://kafka.apache.org/documentation/#brokerconfigs>> > > > > >> <https://kafka.apache.org/documentation/#brokerconfigs > <https://kafka.apache.org/documentation/#brokerconfigs> > > > <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>>> > > > > >> > > <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>>>> > > > > >> > > > > > < > > 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>>> > > > > >> > > <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>>> > > > > >> > > <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>>>> > > > > >> > > > > > <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>>> > > > > >> > > <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 > > > > > > > > > >
