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