ijuma commented on a change in pull request #8701:
URL: https://github.com/apache/kafka/pull/8701#discussion_r429551613



##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -180,6 +180,17 @@ object RollParams {
   }
 }
 
+sealed trait LogStartOffsetIncrementCause

Review comment:
       Nit: LogStartOffsetIncrementReason?

##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -1275,8 +1286,8 @@ class Log(@volatile private var _dir: File,
       lock synchronized {
         checkIfMemoryMappedBufferClosed()
         if (newLogStartOffset > logStartOffset) {
-          info(s"Incrementing log start offset to $newLogStartOffset")
           updateLogStartOffset(newLogStartOffset)
+          info(s"Incremented log start offset to $newLogStartOffset after 
$cause")

Review comment:
       Nit: would `due` be better than `after` here?

##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -180,6 +180,17 @@ object RollParams {
   }
 }
 
+sealed trait LogStartOffsetIncrementCause
+case object ClientRecordDeletion extends LogStartOffsetIncrementCause {
+  override def toString: String = "client delete records request"
+}
+case object LeaderOffsetIncremented extends LogStartOffsetIncrementCause {
+  override def toString: String = "leader offset increment"
+}
+case object SegmentDeletion extends LogStartOffsetIncrementCause {
+  override def toString: String = "segment deletion to enforce retention"

Review comment:
       Maybe we should just say segment deletion? We can also delete segments 
during recovery and such. Could that not trigger this path?




----------------------------------------------------------------
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


Reply via email to