Thank you for comments. I will use '0' for now. If we create topics through admin client in the future, we might perform some useful checks. (but the assumption is all brokers in the same cluster have the same default configurations value, otherwise,it might still be tricky to do such cross validation check.)
Xiongqi (Wesley) Wu On Mon, Sep 10, 2018 at 11:15 AM Colin McCabe <cmcc...@apache.org> wrote: > I don't have a strong opinion. But I think we should probably be > consistent with how segment.ms works, and just use 0. > > best, > Colin > > > On Wed, Sep 5, 2018, at 21:19, Brett Rann wrote: > > 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 > > > > > > > > > > > > > > > > > > >
