Uploaded image for project: 'Kafka'
  1. Kafka
  2. KAFKA-1944

Rename LogCleaner and related classes to LogCompactor

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

      Following a mailing list discussion:
      "the name LogCleaner is seriously misleading. Its more of a log compactor.
      Deleting old logs happens elsewhere from what I've seen."

      Note that this may require renaming related classes, objects, configs and metrics.

        Issue Links

          Activity

          Hide
          omkreddy Manikumar added a comment -

          Can we wait till we check-in KAFKA-1374? So that i can avoid some rebases

          Show
          omkreddy Manikumar added a comment - Can we wait till we check-in KAFKA-1374 ? So that i can avoid some rebases
          Hide
          jjkoshy Joel Koshy added a comment -

          Sure - I think this can wait until that is in.

          Show
          jjkoshy Joel Koshy added a comment - Sure - I think this can wait until that is in.
          Hide
          singhashish Ashish Singh added a comment -

          Sure.

          Show
          singhashish Ashish Singh added a comment - Sure.
          Hide
          omkreddy Manikumar added a comment -

          We have checked-in KAFKA-1374. Now we can rename the LogCleaner related classes.

          Show
          omkreddy Manikumar added a comment - We have checked-in KAFKA-1374 . Now we can rename the LogCleaner related classes.
          Hide
          singhashish Ashish Singh added a comment -

          Manikumar thanks for the info. It's on my todo list, will get to it soon.

          Show
          singhashish Ashish Singh added a comment - Manikumar thanks for the info. It's on my todo list, will get to it soon.
          Hide
          singhashish Ashish Singh added a comment -

          Aravind Selvan all yours. This will probably require a KIP, I can help you with it.

          Show
          singhashish Ashish Singh added a comment - Aravind Selvan all yours. This will probably require a KIP, I can help you with it.
          Hide
          becket_qin Jiangjie Qin added a comment -

          Ashish Singh, LogCleaner is not a public API. Are you referring to the configuration name change?

          Show
          becket_qin Jiangjie Qin added a comment - Ashish Singh , LogCleaner is not a public API. Are you referring to the configuration name change?
          Hide
          singhashish Ashish Singh added a comment -

          Ashish Singh, LogCleaner is not a public API. Are you referring to the configuration name change?

          Jiangjie Qin yes.

          Show
          singhashish Ashish Singh added a comment - Ashish Singh, LogCleaner is not a public API. Are you referring to the configuration name change? Jiangjie Qin yes.
          Hide
          jjkoshy Joel Koshy added a comment -

          Perhaps KIP would be overkill for this. I would recommend just keeping the old config name and add the new config. If the user specifies the old config emit a warning that it will be deprecated.

          Show
          jjkoshy Joel Koshy added a comment - Perhaps KIP would be overkill for this. I would recommend just keeping the old config name and add the new config. If the user specifies the old config emit a warning that it will be deprecated.
          Hide
          pranav.maniar Pranav Maniar added a comment -

          Is someone working on this?? If not I can take it up.
          I am new to kafka and this seems like a refactoring task, which will be easy to start with..

          P.S. : This issue seems to be old, so this still needs to be refactored ??

          Show
          pranav.maniar Pranav Maniar added a comment - Is someone working on this?? If not I can take it up. I am new to kafka and this seems like a refactoring task, which will be easy to start with.. P.S. : This issue seems to be old, so this still needs to be refactored ??
          Hide
          hachikuji Jason Gustafson added a comment -

          The name LogCleaner is somewhat entrenched. If this is just a matter of renaming LogCleaner to LogCompactor, that could be done in a MINOR PR, but that probably only makes sense if we change the configuration log.cleaner.enable as well. On the other hand, since we have changed its default value to true and since two key components now depend on it (i.e. the new consumer and the transactional producer), maybe we should consider deprecating the config instead?

          Show
          hachikuji Jason Gustafson added a comment - The name LogCleaner is somewhat entrenched. If this is just a matter of renaming LogCleaner to LogCompactor , that could be done in a MINOR PR, but that probably only makes sense if we change the configuration log.cleaner.enable as well. On the other hand, since we have changed its default value to true and since two key components now depend on it (i.e. the new consumer and the transactional producer), maybe we should consider deprecating the config instead?
          Hide
          becket_qin Jiangjie Qin added a comment -

          I agree with Jason Gustafson that we can deprecate log.cleaner.enable. The name "log.cleaner" appears in a handful of other configurations related to log cleaner. We will need to change those following the deprecation process as well. It would likely be a quick KIP.

          Show
          becket_qin Jiangjie Qin added a comment - I agree with Jason Gustafson that we can deprecate log.cleaner.enable . The name "log.cleaner" appears in a handful of other configurations related to log cleaner. We will need to change those following the deprecation process as well. It would likely be a quick KIP.
          Hide
          pranav.maniar Pranav Maniar added a comment -

          Ok... So I will start working on it.
          Jiangjie Qin , Jason Gustafson ,
          I don't have permission to assign JIRA to me, can you please assign to me.

          Also, I am new here.. so for KIP creating I will require help .. or If you could create KIP that would be great...

          Show
          pranav.maniar Pranav Maniar added a comment - Ok... So I will start working on it. Jiangjie Qin , Jason Gustafson , I don't have permission to assign JIRA to me, can you please assign to me. Also, I am new here.. so for KIP creating I will require help .. or If you could create KIP that would be great...
          Hide
          becket_qin Jiangjie Qin added a comment -

          Thanks Pranav Maniar. You can follow the instructions in the following link to create a KIP. Let us know if you have any questions.
          https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals

          Show
          becket_qin Jiangjie Qin added a comment - Thanks Pranav Maniar . You can follow the instructions in the following link to create a KIP. Let us know if you have any questions. https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals
          Hide
          pranav.maniar Pranav Maniar added a comment -

          Thanks Jiangjie Qin , I will go through wiki page and try to create KIP.
          I see that I can't access KIP-template. If you can assign permission it will be great. Meanwhile I will also send mail to dev@kafka.apache.org as mentioned on wiki page.

          Show
          pranav.maniar Pranav Maniar added a comment - Thanks Jiangjie Qin , I will go through wiki page and try to create KIP. I see that I can't access KIP-template. If you can assign permission it will be great. Meanwhile I will also send mail to dev@kafka.apache.org as mentioned on wiki page.
          Hide
          guozhang Guozhang Wang added a comment -

          Also agree with Jason Gustafson that we should consider deprecating the config (but not renaming it instantaneously) and introduce a new config, along with the class name changes. In addition, the values taken should be considered; I'm thinking that:

          0. Default value of the new config should be the same as the deprecated config, i.e. true
          1. If both deprecated and new config values are specified, log a WARN entry and choose the new config value.
          2. If only one of them is specified, choose its value.
          3. If none of them are specified, then the default value, which should be the same, will be used.

          Show
          guozhang Guozhang Wang added a comment - Also agree with Jason Gustafson that we should consider deprecating the config (but not renaming it instantaneously) and introduce a new config, along with the class name changes. In addition, the values taken should be considered; I'm thinking that: 0. Default value of the new config should be the same as the deprecated config, i.e. true 1. If both deprecated and new config values are specified, log a WARN entry and choose the new config value. 2. If only one of them is specified, choose its value. 3. If none of them are specified, then the default value, which should be the same, will be used.
          Hide
          ijuma Ismael Juma added a comment -

          By the way, we should also consider whether this change is worth it. After all, many are used to the existing names. Is the improvement worth the disruption?

          Show
          ijuma Ismael Juma added a comment - By the way, we should also consider whether this change is worth it. After all, many are used to the existing names. Is the improvement worth the disruption?
          Hide
          hachikuji Jason Gustafson added a comment -

          I personally don't think it's worth introducing alternative configs just so we can rename classes internally. The naming is unfortunate, but many users have gotten used to it and having multiple config names seems rather annoying (deprecation is a slow process). It sounds like we are in agreement on deprecating log.cleaner.enable? Maybe we should do that in a separate JIRA/KIP and consider closing this as "won't fix"?

          Show
          hachikuji Jason Gustafson added a comment - I personally don't think it's worth introducing alternative configs just so we can rename classes internally. The naming is unfortunate, but many users have gotten used to it and having multiple config names seems rather annoying (deprecation is a slow process). It sounds like we are in agreement on deprecating log.cleaner.enable ? Maybe we should do that in a separate JIRA/KIP and consider closing this as "won't fix"?
          Hide
          pranav.maniar Pranav Maniar added a comment - - edited

          It seems that there are agreement/disagreement related to change...
          So should I create KIP or first we discuss it over JIRA/mailing list ?

          Also, what about other cleaner configs apart from log.cleaner.enable ? Does any of the other cleaner config also requires renaming ? E.g.

          log.cleaner.backoff.ms
          log.cleaner.delete.retention.ms
          log.cleaner.min.compaction.lag.ms
          log.cleaner.threads
          ...
          
          Show
          pranav.maniar Pranav Maniar added a comment - - edited It seems that there are agreement/disagreement related to change... So should I create KIP or first we discuss it over JIRA/mailing list ? Also, what about other cleaner configs apart from log.cleaner.enable ? Does any of the other cleaner config also requires renaming ? E.g. log.cleaner.backoff.ms log.cleaner.delete.retention.ms log.cleaner.min.compaction.lag.ms log.cleaner.threads ...
          Hide
          ijuma Ismael Juma added a comment -

          I agree that we should probably deprecate and eventually remove log.cleaner.enable. It doesn't seem like it ever makes sense to disable it these days.

          Regarding the other configs, I think it makes sense to start a discussion in the dev mailing list before doing the KIP.

          Show
          ijuma Ismael Juma added a comment - I agree that we should probably deprecate and eventually remove log.cleaner.enable. It doesn't seem like it ever makes sense to disable it these days. Regarding the other configs, I think it makes sense to start a discussion in the dev mailing list before doing the KIP.
          Hide
          pranav.maniar Pranav Maniar added a comment -

          As discussed on Mailing List and KIP-184 discussion.
          This name changes will lead to many configuration name changes. Which seems to be unnecessary trouble for users.
          Also there are no evidence of users getting confused because of the naming.
          So it is decided that we will stick with the existing names

          Show
          pranav.maniar Pranav Maniar added a comment - As discussed on Mailing List and KIP-184 discussion. This name changes will lead to many configuration name changes. Which seems to be unnecessary trouble for users. Also there are no evidence of users getting confused because of the naming. So it is decided that we will stick with the existing names

            People

            • Assignee:
              pranav.maniar Pranav Maniar
              Reporter:
              gwenshap Gwen Shapira
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development