kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jason Gustafson <ja...@confluent.io>
Subject Re: [DISCUSS] KIP-184 Rename LogCleaner and related classes to LogCompactor
Date Tue, 22 Aug 2017 22:07:40 GMT
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 <cmccabe@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 <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