Samrat002 commented on code in PR #27187:
URL: https://github.com/apache/flink/pull/27187#discussion_r2930560860
##########
flink-filesystems/flink-s3-fs-native/src/main/java/org/apache/flink/fs/s3native/NativeS3FileSystem.java:
##########
@@ -427,13 +428,8 @@ public FSDataOutputStream create(Path path, WriteMode
overwriteMode) throws IOEx
}
}
- final String key = NativeS3AccessHelper.extractKey(path);
- return new NativeS3OutputStream(
- clientProvider.getS3Client(),
- bucketName,
- key,
- localTmpDir,
- clientProvider.getEncryptionConfig());
+ RecoverableWriter recoverable = createRecoverableWriter();
Review Comment:
No leak here. Here's why:
1. RecoverableWriter is a lightweight factory, not a resource holder
- Contains only configuration and shared references
- close() is a no-op (just sets a flag)
- No S3 connections, file handles, or threads
2. All actual S3 resources are in the returned FSDataOutputStream
- S3 multipart upload ID
- Temporary file handles
- Buffers
3. The lifecycle
- create() returns FSDataOutputStream to caller
- Caller must close the stream (Flink contract)
- RecoverableWriter is a local variable, immediately eligible for GC
If you want to be explicit about cleanup, we could add try-with-resources:
```
try (RecoverableWriter recoverable = createRecoverableWriter()) {
return recoverable.open(path);
}
```
But it's unnecessary since close() is a no-op. The current code looks
correct to me.
Do you see any gap or senerio in mind where leak can happen ?
--
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]
