hachikuji commented on a change in pull request #8850:
URL: https://github.com/apache/kafka/pull/8850#discussion_r438997622
##########
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:
Can we keep the reason? On second thought, maybe it's fine to leave this
as is.
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1794,20 +1811,29 @@ 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" +
Review comment:
In addition, it might be useful to know the total log size. Maybe we
could include `size - diff` as the size after deletion?
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1784,8 +1785,24 @@ 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. Largest record timestamp of segment is
$ts")
Review comment:
nit: could we make the connection clearer? How about this?
```scala
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 is $ts")
```
Also, we seem to have lost mention o
----------------------------------------------------------------
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
