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


##########
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));
+
+            for (RemoteLogSegmentMetadata segmentMetadata : segmentsToDelete) {
+                
remoteLogRetentionHandler.deleteRemoteLogSegment(segmentMetadata, x -> true);

Review Comment:
   Can we `and` the result of 
`remoteLogRetentionHandler.deleteRemoteLogSegment` for all the segments and log 
an `error` statement if it is not able to delete the segments?
   
   ```
   remoteLogRetentionHandler.deleteRemoteLogSegment(segmentMetadata, x -> 
!isCancelled() && isLeader());
   ```



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