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

[via GitHub]

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


##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -1006,6 +1005,10 @@ private void cleanupExpiredRemoteLogSegments() throws 
RemoteStorageException, Ex
 
             // Update log start offset with the computed value after retention 
cleanup is done
             remoteLogRetentionHandler.logStartOffset.ifPresent(offset -> 
handleLogStartOffsetUpdate(topicIdPartition.topicPartition(), offset));

Review Comment:
   nit: Can we move Line 1007-1011 to 985? The 
`deleteLogSegmentsDueToLeaderEpochCacheTruncation` should run only for case 
when there is a truncation in the leader-epoch-checkpoint file which means 
log-start-offset was already moved/updated. 
   
   ```java
   // Update log start offset with the computed value after retention cleanup 
is done
   remoteLogRetentionHandler.logStartOffset.ifPresent(offset -> 
handleLogStartOffsetUpdate(topicIdPartition.topicPartition(), offset));
   for (RemoteLogSegmentMetadata segmentMetadata : segmentsToDelete) { 
       remoteLogRetentionHandler.deleteRemoteLogSegment(segmentMetadata, x -> 
true);
   }
   ```



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