[GitHub] [kafka] clolov commented on a diff in pull request #14349: KAFKA-15352: Update log-start-offset before initiating deletion of remote segments

[via GitHub]

clolov commented on code in PR #14349:
URL: https://github.com/apache/kafka/pull/14349#discussion_r1321614405


##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -1004,19 +999,25 @@ private void cleanupExpiredRemoteLogSegments() throws 
RemoteStorageException, Ex
                     // remote log segments won't be removed. The 
`isRemoteSegmentWithinLeaderEpoch` validates whether
                     // the epochs present in the segment lies in the 
checkpoint file. It will always return false
                     // since the checkpoint file was already truncated.
-                    boolean isSegmentDeleted = 
remoteLogRetentionHandler.deleteLogStartOffsetBreachedSegments(
+                    boolean shouldDeleteSegment = 
remoteLogRetentionHandler.deleteLogStartOffsetBreachedSegments(
                             metadata, logStartOffset, epochWithOffsets);
+                    if (shouldDeleteSegment) {
+                        segmentsToDelete.add(metadata);
+                    }
                     boolean isValidSegment = false;
-                    if (!isSegmentDeleted) {
+                    if (!shouldDeleteSegment) {
                         // check whether the segment contains the required 
epoch range with in the current leader epoch lineage.
                         isValidSegment = 
isRemoteSegmentWithinLeaderEpochs(metadata, logEndOffset, epochWithOffsets);
                         if (isValidSegment) {
-                            isSegmentDeleted =
+                            shouldDeleteSegment =
                                     
remoteLogRetentionHandler.deleteRetentionTimeBreachedSegments(metadata) ||
                                             
remoteLogRetentionHandler.deleteRetentionSizeBreachedSegments(metadata);
+                            if (shouldDeleteSegment) {
+                                segmentsToDelete.add(metadata);
+                            }

Review Comment:
   Thanks for the spot! Hopefully addressed in the next commit



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