I think we need a KIP for config deprecations. The case for this one seems clear-cut since we strongly depend on the log cleaner for the management of consumer offsets and the transaction log, but we can see what others think.
-Jason On Fri, Aug 25, 2017 at 10:00 AM, Pranav Maniar <pranav9...@gmail.com> wrote: > Yes I can take it up deprecation of "log.cleaner.enable" > > Will it require KIP ? > Since as per my understanding we will be honoring value set for > "log.cleaner.enable" till the time it is around. For now just a warning > message about deprecation will be logged only. > > Or should we remove the cofig now only? > > > Thanks, > Pranav > > On Wed, Aug 23, 2017 at 3:37 AM, Jason Gustafson <ja...@confluent.io> > wrote: > > > Hi Pranav, > > > > Yeah, I'd recommend closing it since the benefit is unclear and since no > > one has jumped in to offer stronger support for the change. Were you > > planning to do a KIP to deprecate `log.cleaner.enable`? I still think > that > > makes sense. > > > > Thanks, > > Jason > > > > On Tue, Aug 22, 2017 at 1:47 PM, Colin McCabe <cmcc...@apache.org> > wrote: > > > > > Hmm. There are a lot of configuration keys that involve "log cleaner." > > > It seems like if we rename this component, logically we'd have to > rename > > > all of them and support the old versions as deprecated config keys: > > > > > > val LogCleanupPolicyProp = "log.cleanup.policy" > > > val LogCleanerThreadsProp = "log.cleaner.threads" > > > val LogCleanerIoMaxBytesPerSecondProp = > > > "log.cleaner.io.max.bytes.per.second" > > > val LogCleanerDedupeBufferSizeProp = "log.cleaner.dedupe.buffer. > size" > > > val LogCleanerIoBufferSizeProp = "log.cleaner.io.buffer.size" > > > val LogCleanerDedupeBufferLoadFactorProp = > > > "log.cleaner.io.buffer.load.factor" > > > val LogCleanerBackoffMsProp = "log.cleaner.backoff.ms" > > > val LogCleanerMinCleanRatioProp = "log.cleaner.min.cleanable.ratio" > > > val LogCleanerEnableProp = "log.cleaner.enable" > > > val LogCleanerDeleteRetentionMsProp = > > > "log.cleaner.delete.retention.ms" > > > val LogCleanerMinCompactionLagMsProp = > > > "log.cleaner.min.compaction.lag.ms" > > > > > > This seems like it would be quite painful to users, since they'd have > to > > > deal with deprecation warnings and multiple names for the same > > > configuration. In general I think Jason and Ismael's point is valid: > do > > > we have evidence that "log cleaner" is causing confusion? If not, it > > > may not be worth it to rename this at the moment. > > > > > > regards, > > > Colin > > > > > > > > > On Mon, Aug 21, 2017, at 05:19, Pranav Maniar wrote: > > > > Hi Jason, > > > > > > > > Haven't heard from other on this KIP. Should I close it ? > > > > > > > > ~Pranav > > > > > > > > On Thu, Aug 10, 2017 at 12:04 AM, Jason Gustafson < > ja...@confluent.io> > > > > wrote: > > > > > > > > > Hey Pranav, > > > > > > > > > > Let's see what others think before closing the KIP. If there are > > strong > > > > > reasons for the renaming, I would reconsider. > > > > > > > > > > As far as deprecating `log.cleaner.enable`, I think it's a good > idea > > > and > > > > > can be done in a separate KIP. Guozhang's suggestion seems > > reasonable, > > > but > > > > > I'd just turn it on always (it won't cause much harm if there are > no > > > topics > > > > > enabled for compaction). This is an implementation detail which > > > probably > > > > > doesn't need to be included in the KIP. > > > > > > > > > > -Jason > > > > > > > > > > On Wed, Aug 9, 2017 at 10:47 AM, Pranav Maniar < > pranav9...@gmail.com > > > > > > > > wrote: > > > > > > > > > > > Thanks Ismael, Jason for the suggestion. > > > > > > My bad. I should have followed up on mail-list discussion before > > > starting > > > > > > KIP. Apologies. > > > > > > > > > > > > I am relatively new, so I do not know if any confusion was > reported > > > in > > > > > past > > > > > > due to terminology. May be others can chime in. > > > > > > If the old naming is fine with majority then no changes will be > > > needed. I > > > > > > will mark JIRA as wont'fix and close the KIP ! > > > > > > > > > > > > Ismael, Jason, > > > > > > There was another suggestion from Guozhang on deprecating and > > > eventually > > > > > > removing log.cleaner.enable property all together and always > > > enabling log > > > > > > cleaner if "log.cleanup.policy=compact". > > > > > > What are your suggestion on this ? > > > > > > > > > > > > > > > > > > Thanks, > > > > > > Pranav > > > > > > > > > > > > On Wed, Aug 9, 2017 at 10:27 PM, Jason Gustafson < > > ja...@confluent.io > > > > > > > > > > wrote: > > > > > > > > > > > > > Yes, as Ismael noted above, I am not fond of this renaming. > Keep > > in > > > > > mind > > > > > > > that the LogCleaner does not only handle compaction. It is > > > possible to > > > > > > > configure a cleanup policy of "compact" and "delete," in which > > > case the > > > > > > > LogCleaner also handles removal of old segments. Hence the more > > > general > > > > > > > LogCleaner name is more appropriate in my opinion. > > > > > > > > > > > > > > -Jason > > > > > > > > > > > > > > On Wed, Aug 9, 2017 at 9:49 AM, Pranav Maniar < > > > pranav9...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Thanks Ewen for the suggestions. > > > > > > > > I have updated KIP-184. Updates done are : > > > > > > > > > > > > > > > > 1. If deprecated property is encountered currently, then its > > > value > > > > > will > > > > > > > be > > > > > > > > considered while enabling compactor. > > > > > > > > 2. log.compactor.min.compaction.lag.ms updated it to be > > > > > > > > log.compactor.min.lag.ms ( Other naming suggestions are also > > > > > welcomed) > > > > > > > > 3. Removed implementation details from KIP > > > > > > > > > > > > > > > > ~Pranav > > > > > > > > > > > > > > > > On Wed, Aug 9, 2017 at 11:19 AM, Ewen Cheslack-Postava < > > > > > > > e...@confluent.io> > > > > > > > > wrote: > > > > > > > > > > > > > > > >> A simple log message is standard, but the KIP should > probably > > > > > specify > > > > > > > what > > > > > > > >> happens when the deprecated config is encountered. > > > > > > > >> > > > > > > > >> Other than that, the change LGTM. Other things that might be > > > worth > > > > > > > >> addressing > > > > > > > >> > > > > > > > >> * log.compactor.min.compaction.lag.ms seems a bit redundant > > > with > > > > > > > >> compactor > > > > > > > >> and compaction. Not sure if we'd want to tweak the new > > version. > > > > > > > >> * The class renaming doesn't even need to be in the KIP as > it > > > is an > > > > > > > >> implementation detail. > > > > > > > >> > > > > > > > >> -Ewen > > > > > > > >> > > > > > > > >> On Tue, Aug 8, 2017 at 10:17 PM, Pranav Maniar < > > > > > pranav9...@gmail.com> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >> > Thanks Guozhang for the suggestion. > > > > > > > >> > > > > > > > > >> > For now, I have updated KIP incorporating your suggestion. > > > > > > > >> > Personally I think implicitly enabling compaction whenever > > > policy > > > > > is > > > > > > > >> set to > > > > > > > >> > compact is more appropriate. Because new users like me > will > > > always > > > > > > > >> assume > > > > > > > >> > that setting policy to compact will enable compaction. > > > > > > > >> > > > > > > > > >> > But having said that, It will be interesting to know, if > > > there are > > > > > > any > > > > > > > >> > use-cases where user would explicitly want to turn off the > > > > > > compactor. > > > > > > > >> > > > > > > > > >> > Thanks, > > > > > > > >> > Pranav > > > > > > > >> > > > > > > > > >> > On Tue, Aug 8, 2017 at 2:20 AM, Guozhang Wang < > > > wangg...@gmail.com > > > > > > > > > > > > > >> wrote: > > > > > > > >> > > > > > > > > >> > > Thanks for the KIP proposal, > > > > > > > >> > > > > > > > > > >> > > I thought one suggestion before this discussion is to > > > deprecate > > > > > > the > > > > > > > " > > > > > > > >> > > log.cleaner.enable" and always turn on compaction for > > those > > > > > topics > > > > > > > >> that > > > > > > > >> > > have compact policies? > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > Guozhang > > > > > > > >> > > > > > > > > > >> > > On Sat, Aug 5, 2017 at 9:36 AM, Pranav Maniar < > > > > > > pranav9...@gmail.com > > > > > > > > > > > > > > > >> > > wrote: > > > > > > > >> > > > > > > > > > >> > > > Hi All, > > > > > > > >> > > > > > > > > > > >> > > > Following a discussion on JIRA KAFKA-1944 > > > > > > > >> > > > <https://issues.apache.org/jira/browse/KAFKA-1944> . > I > > > have > > > > > > > created > > > > > > > >> > > > KIP-184 > > > > > > > >> > > > <https://cwiki.apache.org/ > confluence/display/KAFKA/KIP- > > > > > > > >> > > > 184%3A+Rename+LogCleaner+and+related+classes+to+ > > > LogCompactor> > > > > > > > >> > > > as > > > > > > > >> > > > it will require configuration change. > > > > > > > >> > > > > > > > > > > >> > > > As per the process I am starting Discussion on mail > > > thread for > > > > > > > >> KIP-184. > > > > > > > >> > > > > > > > > > > >> > > > Renaming of configuration "log.cleaner.enable" is > > > discussed on > > > > > > > >> > > KAFKA-1944. > > > > > > > >> > > > But other log.cleaner configuration also seems to be > > used > > > by > > > > > > > cleaner > > > > > > > >> > > only. > > > > > > > >> > > > So to maintain naming consistency, I have proposed to > > > rename > > > > > all > > > > > > > >> these > > > > > > > >> > > > configuration. > > > > > > > >> > > > > > > > > > > >> > > > Please provide your suggestion/views for the same. > > Thanks > > > ! > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > Thanks, > > > > > > > >> > > > Pranav > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > -- > > > > > > > >> > > -- Guozhang > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
