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 > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > >
