-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
