LuciferYang opened a new pull request, #9655: URL: https://github.com/apache/paimon/pull/9655
### Purpose close #9650 `FormatTableSingleFileWriter` asks its `FileIO` for a two-phase output stream. `ResolvingFileIO` forwards `newOutputStream` and `tryToWriteAtomic` to the FileIO that owns the scheme, but not `newTwoPhaseOutputStream`, so that call fell through to the interface default and produced a `RenamingTwoPhaseOutputStream` on the resolver itself. On an object store a rename is a server-side copy plus delete, so the whole file was copied a second time at commit, the commit was not atomic, and a `_temporary` directory was left behind. OSS, S3 and Jindo all override the method with a native multipart-upload commit, and the sibling wrapper `RESTTokenFileIO` already forwards it. Forwarding alone is not enough, which is what the issue asked about. `OSSMultiPartUploadCommitter`, `S3MultiPartUploadCommitter` and the Jindo one each cast the FileIO they are handed at commit time to their own concrete type, and `FormatTableCommit` commits with the table's FileIO, which is the resolver. So this also resolves in `BaseMultiPartUploadCommitter.multiPartUploadStore(FileIO)`, right after the `RESTTokenFileIO` unwrap that is already there. `commit`, `discard` and `discardStaging` all route through that private method, so one resolve covers the three paths and all three cloud committers. Of the three options I listed in the issue this is the one that matches the existing precedent in that class; happy to move it if you prefer the resolution to happen in `FormatTableCommit` instead. The unwrap order is `RESTTokenFileIO` then `ResolvingFileIO`, and it does not need to be a loop: `ResolvingFileIO.fileIO(Path)` ends in `FileIO.get`, which only ever loads by scheme, so it cannot produce a `RESTTokenFileIO`, while the reverse nesting does occur. `@VisibleForTesting` comes off `ResolvingFileIO.fileIO(Path)` since production code now calls it. `RESTTokenFileIO.fileIO()`, which the same method already calls, is a plain public method. One gap this does not close: when the local cache is enabled, `AbstractCatalog` wraps the table's FileIO in a `CachingFileIO`, and that class does not forward `newTwoPhaseOutputStream` either, so those writes still get the rename-based stream. Closing it means adding the forwarding there, exposing that class's delegate for the commit-side resolve, and deciding how the cache layer should behave across a two-phase commit, which is more than this fix. In the default configuration `wrapWithCachingIfNeeded` returns the FileIO unchanged, so the resolver is the table's FileIO and this change takes effect. ### Tests `ResolvingFileIOTest.testNewTwoPhaseOutputStreamReachesResolvedOverride` configures a resolver with a mock loader for `oss` and asserts the returned stream is the one the resolved FileIO produced, and that nothing was renamed. New `BaseMultiPartUploadCommitterTest` uses a `BaseMultiPartUploadCommitter` subclass that records the FileIO it is asked to build a store from. `testCommitResolvesResolvingFileIO` and `testDiscardStagingResolvesResolvingFileIO` assert that a resolver never reaches the subclass, which is where the `ClassCastException` would happen; `testConcreteFileIOIsPassedThroughUnchanged` pins that a FileIO needing no resolution is passed through as-is. With both main files reverted to master, the three assertions fail (`expected: Mock for TwoPhaseOutputStream but was: RenamingTwoPhaseOutputStream`, and both commit-side ones on the resolver identity). With the change in place the two classes pass 11/11. -- 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]
