mateczagany commented on PR #29132: URL: https://github.com/apache/flink/pull/29132#issuecomment-5617604560
Yes, removing abort from `close()` does help and leaves `FLINK-39786`’s `closeForCommit()` failure handling unchanged. However, aborting in `closeForCommit()` when a completed checkpoint already references the upload, even in the catch block can cause further issues with FileSink: - The sink uploads a full part, e.g. 5 MiB, under upload ID U1 - A checkpoint completes while the file remains open. snapshotState() calls persist() (FileWriterBucket.java:217), saving a recoverable referencing U1 - More records arrive, leaving a small tail buffered locally - The rolling policy triggers closing the part file (FileWriterBucket.java:231), reaching closeForCommit() - Uploading that final tail fails with an S3 service error after retries. For example, HTTP 503. The error becomes an IOException, and the failure handler aborts U1 (flink-filesystems/flink-s3-fs-native/src/main/java/org/apache/flink/fs/s3native/writer/NativeS3RecoverableFsDataOutputStream.java:235). The abort succeeds - The task restarts from the completed checkpoint. Recovery reuses U1; its next part upload fails because U1 was aborted. S3 reports `NoSuchUpload` So the full fix would be either to: - Keep the work of `FLINK-39786` and add flag `uploadMayBeReferenced` that's checked in `closeForCommit()`. If it's true, don't abort MPU. We don't have to use the flag in `close()`, we can simply always skip abort there - Simply never abort MPUs, but I feel like that would go against the spirit of `FLINK-39786`. This is the path I would have chosen before I saw that ticket -- 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]
