showuon commented on code in PR #16959:
URL: https://github.com/apache/kafka/pull/16959#discussion_r1730655484


##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -1254,6 +1255,20 @@ void cleanupExpiredRemoteLogSegments() throws 
RemoteStorageException, ExecutionE
                         canProcess = false;
                         continue;
                     }
+
+                    if 
(RemoteLogSegmentState.COPY_SEGMENT_STARTED.equals(metadata.state())) {
+                        // get the current segment state here to avoid the 
race condition that before the loop, it's under copying process,
+                        // but then completed. In this case, 
segmentIdsBeingCopied will not contain this id, so we might
+                        // delete this segment unexpectedly.
+                        Optional<RemoteLogSegmentMetadata> curMetadata = 
remoteLogMetadataManager.remoteLogSegmentMetadata(

Review Comment:
   > Instead of deleting the dangling segments in the same iteration, Can we 
note down the dangling segments in the current iteration and delete them in the 
next iteration of cleanupExpiredRemoteLogSegments?
   
   This is a good suggestion! Let me update the PR.



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

Reply via email to