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