-
Notifications
You must be signed in to change notification settings - Fork 14.7k
MINOR: Followup KAFKA-19112 document updated #20492
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
base: trunk
Are you sure you want to change the base?
Conversation
docs/upgrade.html
Outdated
local log segments will be cleaned based on the values of <code>log.local.retention.bytes</code> and | ||
<code>log.local.retention.ms</code>. | ||
<br> | ||
Note that <code>cleanup.policy</code> supports empty values, which means infinite retention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remind users that this use case is equivalent to setting retention.ms=-1
and retention.bytes=-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this paragraph to above line 139?
Note that cleanup.policy
supports empty values => cleanup.policy
now supports empty values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st: LGTM
leaderLogIfLocal match { | ||
case Some(leaderLog) => | ||
if (!leaderLog.config.delete) | ||
if (!leaderLog.config.delete && leaderLog.config.compact) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@junrao WDYT? We have not discussed the support of DeleteRecordsRequest
in the case where cleanup.policy
is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This change seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st please update KIP and upgrade.html to match this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the upgrade.html
and the KIP-1161, PTAL. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st : Thanks for the PR. Just a minor comment.
leaderLogIfLocal match { | ||
case Some(leaderLog) => | ||
if (!leaderLog.config.delete) | ||
if (!leaderLog.config.delete && leaderLog.config.compact) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This change seems reasonable to me.
docs/upgrade.html
Outdated
local log segments will be cleaned based on the values of <code>log.local.retention.bytes</code> and | ||
<code>log.local.retention.ms</code>. | ||
<br> | ||
Note that <code>cleanup.policy</code> supports empty values, which means infinite retention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this paragraph to above line 139?
Note that cleanup.policy
supports empty values => cleanup.policy
now supports empty values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st : Thanks for the updated PR. LGTM
Some sections are not very clear, and we need to update the
documentation.