kamalcph commented on code in PR #14649: URL: https://github.com/apache/kafka/pull/14649#discussion_r1404590243
########## core/src/main/scala/kafka/log/UnifiedLog.scala: ########## @@ -1754,6 +1754,7 @@ class UnifiedLog(@volatile var logStartOffset: Long, leaderEpochCache.foreach(_.clearAndFlush()) producerStateManager.truncateFullyAndStartAt(newOffset) logStartOffset = logStartOffsetOpt.getOrElse(newOffset) + if (remoteLogEnabled()) maybeIncrementLocalLogStartOffset(newOffset, LogStartOffsetIncrementReason.SegmentDeletion) Review Comment: > Adding it to truncateFullyAndStartAt means that anything that calls the function does not have to worry about the updating the local log start offset every time. My preference is to update the local-log-start-offset in `truncateFullyAndStartAt` function as it updates the leader-epoch-checkpoint file, log-start-offset, high-watermark, and log-end-offset. To avoid passing the reason in maybeIncrementLocalLogStartOffset` method, we can update the `local-log-start-offset` value directly in truncateFullyAndStartAt method since it takes the same `lock`. We followed a similar approach in our local branch: https://sourcegraph.com/github.com/satishd/kafka@24d21694f2f0450e8a8e60e85400de3586fc7069/-/blob/core/src/main/scala/kafka/log/Log.scala?L2385 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org