nikramakrishnan commented on code in PR #14649:
URL: https://github.com/apache/kafka/pull/14649#discussion_r1400962913
##########
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:
I have made `maybeIncrementLocalLogStartOffset` public and calling it from
`buildRemoteLogAuxState`.
Although I think that we should still update the local log start offset in
`truncateFullyAndStartAt` because we know **exactly** what the start offset of
the local log is here. The replica fetcher tier state machine already calls
`maybeIncrementLogStartOffset`, but that call is redundant because we already
update the log start offset in `truncateFullyAndStartAt` to the
`logStartOffsetOpt` parameter (without specifying a reason).
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. Although we should not hardcode the reason here because that will
not be always correct. The two options then are (i) update directly like we do
for the log start offset, or (ii) accept the reason as a parameter which means
modify all callers, including lots of `truncateFully...` functions down the
chain, and modify all the places including tests that call any of those
functions.
I will leave this open to discussion. +cc @divijvaidya @kamalcph @satishd
@Hangleton
This PR in its current state is ready to be reviewed.
--
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
