Skip to content

KAFKA-19151: docs: clarify that flush.ms requires log.flush.scheduler.interval.ms config #19479

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.

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

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

yunchipang
Copy link
Contributor

Enhanced docs of flush.ms to remind users the flush is triggered by log.flush.scheduler.interval.ms.

Reviewers: @chia7712

@github-actions github-actions bot added triage PRs from the community clients small Small PRs labels Apr 15, 2025
"this and use replication for durability and allow the operating system's background " +
"we would fsync after 1000 ms had passed. Note that this setting depends on the broker-level " +
"configuration \"log.flush.scheduler.interval.ms\", which controls how frequently the flush check occurs. " +
"If \"log.flush.scheduler.interval.ms\" is not configured, the topic config \"flush.ms\" will not be triggered. " +
Copy link
Member

Choose a reason for hiding this comment

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

nit: use <code></code> instead, so we can have code block for configuration.

Example:
Screenshot 2025-04-16 at 10 17 55 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

"If "log.flush.scheduler.interval.ms" is not configured, the topic config "flush.ms" will not be triggered. "

Maybe we can remove this as the remaining docs is good enough

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also use the LOG_FLUSH_SCHEDULER_INTERVAL_MS_CONFIG and FLUSH_MS_CONFIG constants instead of hardcoding the strings.

Copy link
Member

Choose a reason for hiding this comment

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

@m1a2st The LOG_FLUSH_SCHEDULER_INTERVAL_MS_CONFIG is in org.apache.kafka.server.config.ServerLogConfigs. It already imported TopicConfig. I don't think we can import the ServerLogConfigs configuration in TopicConfig.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The LOG_FLUSH_SCHEDULER_INTERVAL_MS_CONFIG is in org.apache.kafka.server.config.ServerLogConfigs

Thanks, but FLUSH_MS_CONFIG can update.

Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

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

@yunchipang: Thanks for the patch.
Please provide a screenshot of your patch if it is related to documentation.
You can find the instructions in the readme of https://github.com/apache/kafka-site

"this and use replication for durability and allow the operating system's background " +
"we would fsync after 1000 ms had passed. Note that this setting depends on the broker-level " +
"configuration \"log.flush.scheduler.interval.ms\", which controls how frequently the flush check occurs. " +
"If \"log.flush.scheduler.interval.ms\" is not configured, the topic config \"flush.ms\" will not be triggered. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot removed the triage PRs from the community label Apr 16, 2025
@yunchipang
Copy link
Contributor Author

@chia7712 @FrankYang0529 @frankvicky @m1a2st thanks y'all for reviewing. i have pushed the changes, please find the screenshot of the patch below. one newbie question, do i raise a PR in kafka-site repo after this patch is merged?
Screenshot 2025-04-15 at 9 16 20 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants