hachikuji commented on a change in pull request #8850:
URL: https://github.com/apache/kafka/pull/8850#discussion_r441699978
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1784,8 +1785,26 @@ class Log(@volatile private var _dir: File,
private def deleteRetentionMsBreachedSegments(): Int = {
if (config.retentionMs < 0) return 0
val startMs = time.milliseconds
- deleteOldSegments((segment, _) => startMs - segment.largestTimestamp >
config.retentionMs,
- reason = s"retention time ${config.retentionMs}ms breach")
+
+ def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment])
= {
+ if (startMs - segment.largestTimestamp > config.retentionMs) {
+ segment.largestRecordTimestamp match {
+ case Some(ts) =>
+ info(s"Segment with base offset ${segment.baseOffset} will be
deleted due to" +
+ s" retentionMs breach based on the largest record timestamp from
the segment, which" +
Review comment:
We lost the reference to `config.retentionMs`.
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1794,20 +1813,30 @@ class Log(@volatile private var _dir: File,
def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment])
= {
if (diff - segment.size >= 0) {
diff -= segment.size
+ info(s"Segment with base offset ${segment.baseOffset} will be deleted
due to" +
+ s" retentionSize breach. Segment size is ${segment.size} and total
log size after" +
Review comment:
Similarly, can you mention `config.retentionSize`?
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1794,20 +1813,30 @@ class Log(@volatile private var _dir: File,
def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment])
= {
if (diff - segment.size >= 0) {
diff -= segment.size
+ info(s"Segment with base offset ${segment.baseOffset} will be deleted
due to" +
+ s" retentionSize breach. Segment size is ${segment.size} and total
log size after" +
+ s" deletion will be ${size - diff}")
true
} else {
false
}
}
- deleteOldSegments(shouldDelete, reason = s"retention size in bytes
${config.retentionSize} breach")
+ deleteOldSegments(shouldDelete)
}
private def deleteLogStartOffsetBreachedSegments(): Int = {
- def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment]) =
- nextSegmentOpt.exists(_.baseOffset <= logStartOffset)
+ def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment])
= {
+ if (nextSegmentOpt.exists(_.baseOffset <= logStartOffset)) {
+ info(s"Segment with base offset ${segment.baseOffset} will be deleted
due to" +
+ s" startOffset breach. logStartOffset is ${logStartOffset}")
Review comment:
nit: braces unnecessary for simple variable references
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1702,11 +1702,12 @@ class Log(@volatile private var _dir: File,
* (if there is one) and returns true iff it is deletable
* @return The number of segments deleted
*/
- private def deleteOldSegments(predicate: (LogSegment, Option[LogSegment]) =>
Boolean, reason: String): Int = {
+ private def deleteOldSegments(predicate: (LogSegment, Option[LogSegment]) =>
Boolean) = {
Review comment:
In that case, perhaps we can remove the log message? We already have the
following logging when in the `deleteSegments` path:
```scala
private def deleteSegmentFiles(segments: Iterable[LogSegment],
asyncDelete: Boolean): Unit = {
segments.foreach(_.changeFileSuffixes("", Log.DeletedFileSuffix))
def deleteSegments(): Unit = {
info(s"Deleting segments ${segments.mkString(",")}")
maybeHandleIOException(s"Error while deleting segments for
$topicPartition in dir ${dir.getParent}") {
segments.foreach(_.deleteIfExists())
}
}
if (asyncDelete) {
info(s"Scheduling segments for deletion ${segments.mkString(",")}")
scheduler.schedule("delete-file", () => deleteSegments, delay =
config.fileDeleteDelayMs)
} else {
deleteSegments()
}
}
```
So it seems this message is not adding any additional value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
