kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pranav Maniar <pranav9...@gmail.com>
Subject Re: [DISCUSS] KIP-184 Rename LogCleaner and related classes to LogCompactor
Date Mon, 21 Aug 2017 12:19:27 GMT
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 <jason@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 <pranav9428@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 <jason@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 <pranav9428@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 <
> > > ewen@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 <
> pranav9428@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 <wangguoz@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 <
> > pranav9428@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
> > > >> > >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message