KAFKA-13194: bound cleaning by both LSO and HWM when firstUnstableOffsetMetadata is None #11199
Conversation
There was a problem hiding this comment.
@lbradstreet : Thanks for the PR. Great find! Just a couple of minor comments below.
| @@ -595,8 +595,8 @@ private[log] object LogCleanerManager extends Logging { | |||
| // may be cleaned | |||
| val firstUncleanableDirtyOffset: Long = Seq( | |||
|
|
|||
| // we do not clean beyond the first unstable offset | |||
| log.firstUnstableOffset, | |||
| // we do not clean beyond the last stable offset | |||
There was a problem hiding this comment.
This is an existing issue. But could we update the comment in line 593 to include last stable offset too?
There was a problem hiding this comment.
Makes sense. I've updated it with a better explanation of what we bound by.
| val lastCleanOffset = Some(0L) | ||
| val cleanableOffsets = LogCleanerManager.cleanableOffsets(log, lastCleanOffset, time.milliseconds) | ||
| assertEquals(0L, cleanableOffsets.firstDirtyOffset, "The first cleanable offset starts at the beginning of the log.") | ||
| assertEquals(log.highWatermark, cleanableOffsets.firstUncleanableDirtyOffset, "The first uncleanable offset is bounded by the hwm.") |
There was a problem hiding this comment.
Since the description of the test says bounded by LSO, should we change log.highWatermark to log.lastStableOffset and the error message accordingly?
There was a problem hiding this comment.
This is what I intended since the lastStableOffset should equal the highWatermark, but I think it makes sense to keep the intent. Instead I have checked that log.highWatermark == log.lastStableOffset in the line prior to it.
There was a problem hiding this comment.
@lbradstreet : Thanks for the updated PR. LGTM. Are the 29 test failures related to this PR?
@junrao yeah, they're looking related. Let me clean those up. |
There was a problem hiding this comment.
@lbradstreet : Thanks for the updated PR. Were the java 16 tests ok?
@junrao it looks like it is one of the issues where System.exit was called without the Exit.exit override. I don't think it's related to this change but if we want we can rerun it to be sure. |
…setMetadata is None (apache#11199) When the high watermark is contained in a non-active segment, we are not correctly bounding it by the hwm. This means that uncommitted records may overwrite committed data. I've separated out the bounding point tests to check the hwm case in addition to the existing active segment case. Reviewers: Jun Rao <junrao@gmail.com>
When the high watermark is contained in a non-active segment, we are not correctly bounding it by the hwm. This means that uncommitted records may overwrite committed data. I've separated out the bounding point tests to check the hwm case in addition to the existing active segment case.
See https://issues.apache.org/jira/browse/KAFKA-13194 for a further explanation.