Samrat002 commented on PR #29132:
URL: https://github.com/apache/flink/pull/29132#issuecomment-5684414994
Short history, The `close()` abort predates FLINK-39786. It's been there
since the module's initial implementation. FLINK-39786 added the one in
`closeForCommit()`'s catch block, for the case where `uploadCurrentPart()`
throws while `closed` is already true, so the following `close()` no-ops and
nothing aborts. It was a miss from my end.
The reason it was added because In FLIP-555, the idea was to use S3
lifecycle management to clean up orphaned uploads: "a lifecycle rule will get
it eventually". In production, what we observed most buckets don't have
`AbortIncompleteMultipartUpload` configured at all. This was causing
substantial bill at higher scale.
I'm on board with dropping both. Few notes:
- `uploadMayBeReferenced` is a plain field while `closed` is `volatile`, and
the constructor write marking recovered streams isn't under the lock even
though the javadoc allows `close()` from another thread. That's a third hole in
the same invariant.
- The downside is asymmetric. Our `commitAfterRecovery()` is a bare
`commit()`, unlike Hadoop's, which falls back to checking the object exists at
the expected length. A wrong abort then fails every restore of that checkpoint,
and `execution.checkpointing.num-retained` defaults to 1.
We do lose FLINK-39786's case. A commit failure with no recoverable anywhere
will now leak until the lifecycle rule fires. Worst case if bucket has no
configured lifecycle rules. This is Acceptable as native-s3-fs does not claim
to cleanup orphan MPU, We can call this out in documentation.
I am alligned with proposal:
1. the docs should say the lifecycle rule is required, not recommended.
2. +1 to Mate's follow-up ticket for proper orphan cleanup.
--
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]
