-
Notifications
You must be signed in to change notification settings - Fork 14k
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
KAFKA-12177: apply log start offset retention before time and size based retention #10216
Conversation
@junrao, could you please give this a look when you get a chance? |
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.
Nice catch @lbradstreet . +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.
@lbradstreet : Thanks for the PR. Just a minor comment below.
log.maybeIncrementLogStartOffset(1L, ClientRecordDeletion) | ||
assertEquals(2, log.deleteOldSegments(), | ||
"Expecting two segment deletions as log start offset retention should unblock time based retention") | ||
//assertEquals(0, log.deleteOldSegments()) |
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.
Is this still needed?
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.
@lbradstreet : Thanks for the PR. LGTM
* apache-github/trunk: KAFKA-12400: Upgrade jetty to fix CVE-2020-27223 MINOR: Fix null exception in coordinator log (apache#10250) y KAFKA-12375: don't reuse thread.id until a thread has fully shut down (apache#10215) KAFKA-12177: apply log start offset retention before time and size based retention (apache#10216) KAFKA-12369; Implement `ListTransactions` API (apache#10206)
Log start offset retention is the cheapest retention to evaluate and does not require access to maxTimestamp fields for segments, nor segment sizes. In addition, it may unblock other types of retention such as time based retention. Without this change retention is not idempotent. It's possible for one deleteOldSegments call to delete segments due to log start offset retention, and a follow up call to delete due to time based retention, even if the time has not changed.