Skip to content

KAFKA-13610: Deprecate log.cleaner.enable configuration #19472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Apr 16, 2025
Merged

KAFKA-13610: Deprecate log.cleaner.enable configuration #19472

merged 6 commits into from
Apr 16, 2025
+12 −2

Conversation

frankvicky
Copy link
Contributor

@frankvicky frankvicky commented Apr 15, 2025 •
edited by github-actions bot
Loading

JIRA: KAFKA-13610 This patch deprecates the log.cleaner.enable
configuration. It's part of
KIP-1148.

Reviewers: Chia-Ping Tsai [email protected], PoAn Yang
[email protected], Ken Huang [email protected], Jhen-Yung Hsu
[email protected]

@github-actions github-actions bot added triage PRs from the community storage Pull requests that target the storage module small Small PRs labels Apr 15, 2025
@frankvicky
Copy link
Contributor Author

Preview:
image
image

FrankYang0529
FrankYang0529 reviewed Apr 15, 2025
View reviewed changes
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankvicky Thanks for the PR. In KIP-1148, you mentioned that "We should document that users should no longer set this configuration to false after we deprecate it.". Does that mean the LogCleaner will be always enabled after AK 5.0 (remove configuration)? Or it will be always enabled after 4.1 (deprecate configuration)?

Screenshot 2025-04-15 at 6 10 00 PM

BTW, can you also remove unused logCleanerEnable in KafkaConfig? Thanks.

kafka/core/src/main/scala/kafka/server/KafkaConfig.scala

Line 342 in c527530

val logCleanerEnable = getBoolean(CleanerConfig.LOG_CLEANER_ENABLE_PROP)

Yunyung
Yunyung reviewed Apr 15, 2025
View reviewed changes
Copy link
Collaborator

@Yunyung Yunyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Left one small comment.

And follow @FrankYang0529's question, should we mention a bit about the impact of this change and what users should expect in the docs?

storage/src/main/java/org/apache/kafka/storage/internals/log/CleanerConfig.java Outdated
@@ -68,7 +69,8 @@ public class CleanerConfig {
public static final String LOG_CLEANER_DEDUPE_BUFFER_LOAD_FACTOR_DOC = "Log cleaner dedupe buffer load factor. The percentage full the dedupe buffer can become. A higher value " +
"will allow more log to be cleaned at once but will lead to more hash collisions";
public static final String LOG_CLEANER_BACKOFF_MS_DOC = "The amount of time to sleep when there are no logs to clean";
public static final String LOG_CLEANER_ENABLE_DOC = "Enable the log cleaner process to run on the server. Should be enabled if using any topics with a cleanup.policy=compact including the internal offsets topic. If disabled those topics will not be compacted and continually grow in size.";
public static final String LOG_CLEANER_ENABLE_DOC = "This configuration has been deprecated and will be remove in Kafka 5.0. User shouldn't set it to false anymore." +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Add a space at the end of the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a reference to KIP-1148. It should be enough.

@github-actions github-actions bot added the core Kafka Broker label Apr 15, 2025
@frankvicky
Copy link
Contributor Author

Hi @FrankYang0529
This configuration still exists and is configurable, but as KIP said, the user should not set it to false anymore since it will be removed. After AK 5.0, logCleaner will always enable.
I have also updated the KIP content to make it clearer.

m1a2st
m1a2st reviewed Apr 15, 2025
View reviewed changes
Copy link
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, left a comment

storage/src/main/java/org/apache/kafka/storage/internals/log/CleanerConfig.java
@@ -68,7 +69,8 @@ public class CleanerConfig {
public static final String LOG_CLEANER_DEDUPE_BUFFER_LOAD_FACTOR_DOC = "Log cleaner dedupe buffer load factor. The percentage full the dedupe buffer can become. A higher value " +
"will allow more log to be cleaned at once but will lead to more hash collisions";
public static final String LOG_CLEANER_BACKOFF_MS_DOC = "The amount of time to sleep when there are no logs to clean";
public static final String LOG_CLEANER_ENABLE_DOC = "Enable the log cleaner process to run on the server. Should be enabled if using any topics with a cleanup.policy=compact including the internal offsets topic. If disabled those topics will not be compacted and continually grow in size.";
public static final String LOG_CLEANER_ENABLE_DOC = "This configuration has been deprecated and will be removed in Kafka 5.0. Users should not set it to false to prepare for its future removal. " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add @Deprecated on LOG_CLEANER_ENABLE_DOC, the DOC variable also a part of public API, like this PR 4121a89

FrankYang0529
FrankYang0529 approved these changes Apr 15, 2025
View reviewed changes
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

chia7712
chia7712 reviewed Apr 15, 2025
View reviewed changes
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanerConfig.java Outdated
@@ -68,7 +69,9 @@ public class CleanerConfig {
public static final String LOG_CLEANER_DEDUPE_BUFFER_LOAD_FACTOR_DOC = "Log cleaner dedupe buffer load factor. The percentage full the dedupe buffer can become. A higher value " +
"will allow more log to be cleaned at once but will lead to more hash collisions";
public static final String LOG_CLEANER_BACKOFF_MS_DOC = "The amount of time to sleep when there are no logs to clean";
public static final String LOG_CLEANER_ENABLE_DOC = "Enable the log cleaner process to run on the server. Should be enabled if using any topics with a cleanup.policy=compact including the internal offsets topic. If disabled those topics will not be compacted and continually grow in size.";
@Deprecated(since = "4.1")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Deprecated(since = "4.1", forRemoval = true)

@frankvicky frankvicky merged commit 73afcc9 into apache:trunk Apr 16, 2025
24 checks passed
@github-actions github-actions bot removed the triage PRs from the community label Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yunyung Yunyung Yunyung left review comments

@FrankYang0529 FrankYang0529 FrankYang0529 approved these changes

@chia7712 chia7712 chia7712 approved these changes

@m1a2st m1a2st m1a2st approved these changes

Assignees
No one assigned
Labels
core Kafka Broker small Small PRs storage Pull requests that target the storage module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frankvicky @FrankYang0529 @chia7712 @Yunyung @m1a2st