kamalcph commented on code in PR #22661:
URL: https://github.com/apache/kafka/pull/22661#discussion_r3497772896
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
@@ -1440,6 +1454,10 @@ void cleanupExpiredRemoteLogSegments() throws
RemoteStorageException, ExecutionE
// Build the leader epoch map by filtering the epochs that do not
have any records.
NavigableMap<Integer, Long> epochWithOffsets =
buildFilteredLeaderEpochMap(leaderEpochCache.epochWithOffsets());
+ // Seed highestOffsetInRemoteStorage before size-retention to
avoid the fresh-leader double-count
+ // (no-op once RLMCopyTask/RLMFollowerTask have seeded it).
+ maybeSeedHighestOffsetInRemoteStorage(log);
Review Comment:
> (ex: no log uploaded, yet), we won't trigger findHighestRemoteOffset each
time we enter RLMExpirationTask.
This case seem already handled. We have a
[check](https://sourcegraph.com/r/github.com/apache/kafka@trunk/-/blob/storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java?L1427)
to short-circuit when the metadata count is 0.
> we should rely on copiedOffsetOption
To rely on `copiedOffsetOption`, we have to move the variable to RLMTask
that also inherited by the RLMFollowerTask. We may compute the
`findHighestRemoteOffset` twice when both the copy and expiration threads run
concurrently that should be fine as it takes the highest offset. So, I'm fine
with the current approach too.
Thanks for finding and fixing this bug!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
