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