NestDream commented on PR #28360: URL: https://github.com/apache/flink/pull/28360#issuecomment-5147926224
Hi @Samrat002, thanks for the review. The branch is now up to date with master, conflicts resolved. The Files changed tab shows the net change: two files in `flink-s3-fs-native`. While syncing I noticed the `closed = true` ordering issue in `closeForCommit()` that this PR flagged on June 8 was also addressed in FLINK-39110's July revision. Glad it's fixed on master either way. 😀 This PR keeps master's version of that logic as-is and narrows to what remains: - Three `Files.delete()` → `Files.deleteIfExists()` conversions plus removal of the now-redundant `exists()` guard in `close()`, making cleanup idempotent (consistent with `NativeS3RecoverableWriter`). Without this, a temp file already removed from `io.tmp.dirs` turns a healthy commit into a `NoSuchFileException` failure. - Four unit tests for this class's temp-file contract — its first. Two of them pin the current master behavior (stream stays open after a failed commit-time upload; the temp file is intentionally retained for `close()` to reclaim) against regression. `closeForCommitIsIdempotentWhenTempFileMissing` fails without the change; the other three pass on master and serve as pins. When merging, "Squash and merge" lands exactly this diff; the commit message of `e7ae926` is ready to use as the squash message. Thanks! -- 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]
