[python] Use conditional OSS puts for atomic metadata writes - #9715
[python] Use conditional OSS puts for atomic metadata writes#9715wangzhigang1999 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
OssFileIO.try_to_write_atomic derives the OSS object key incorrectly for credential-in-URI OSS forms, which can write metadata to the wrong object path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes concurrent snapshot/metadata commit overwrites for OSS in PyPaimon by introducing an OSS-specific atomic-create path that uses the OSS SDK’s conditional PutObject (x-oss-forbid-overwrite=true) instead of the existing temp-file-and-rename publication flow.
Changes:
- Add
OssFileIO(subclassingPyArrowFileIO) to implement conditional metadata writes viaoss2, with bucket-versioning detection and a legacy fallback. - Route OSS selection and REST token refresh /
ResolvingFileIOatomic writes through the OSS implementation; update internal OSS call sites accordingly. - Add OSS SSE option forwarding for conditional PUTs, extend optional extras to include
oss2, document configuration, and add protocol-level tests.
File summaries
| File | Description |
|---|---|
| paimon-python/setup.py | Adds oss2 to oss and jindo extras for OSS atomic metadata writes. |
| paimon-python/README.md | Documents OSS atomic metadata commits, versioning fallback semantics, and SSE option behavior. |
| paimon-python/pypaimon/tests/py36/ao_simple_test.py | Switches OSS initialization tests to use OssFileIO. |
| paimon-python/pypaimon/tests/oss_legacy_mode_test.py | Updates legacy-mode tests to target OssFileIO while keeping legacy assertions. |
| paimon-python/pypaimon/tests/oss_file_io_test.py | Updates OSS integration-style tests to instantiate OssFileIO. |
| paimon-python/pypaimon/tests/oss_atomic_write_test.py | New protocol tests exercising conditional PUT behavior, SSE headers, failures, and fallback. |
| paimon-python/pypaimon/tests/lance_utils_test.py | Uses FileIO.get(...) for OSS so the correct implementation is selected. |
| paimon-python/pypaimon/tests/file_io_test.py | Updates OSS-related unit tests to use OssFileIO. |
| paimon-python/pypaimon/sample/rest_catalog_blob_as_descriptor_sample.py | Uses FileIO.get(...) for external OSS IO selection. |
| paimon-python/pypaimon/filesystem/resolving_file_io.py | Forwards try_to_write_atomic to the scheme-resolved underlying FileIO. |
| paimon-python/pypaimon/filesystem/oss_file_io.py | Implements OSS conditional atomic creation and SSE header resolution. |
| paimon-python/pypaimon/common/options/config.py | Adds OSS SSE-related config options for atomic metadata PUTs. |
| paimon-python/pypaimon/common/file_io.py | Selects OssFileIO for oss:// URIs in FileIO.get(...). |
| paimon-python/pypaimon/catalog/rest/rest_token_file_io.py | Uses FileIO.get(...) so OSS paths pick up the OSS atomic-write implementation. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Credential-style fallback paths and cross-bucket REST FileIO caching can target or bind to the wrong OSS bucket.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The critical conditional-write test module is skipped in all configured Python CI lanes because oss2 is not installed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
JingsongLi
left a comment
There was a problem hiding this comment.
The conditional publication path addresses the real concurrent-commit data loss in #9714. The current head still has an authentication compatibility issue that can bypass the fix on V4-only OSS accounts/buckets; details are inline.
43 existing protocol/REST/legacy tests passed, and six focused probes checked the real SDK's signed requests and fallback behavior for AK and STS. These used a simulated transport; no live OSS account was exercised.
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: CLEAN in the reviewed scope.
The previous V1 signing finding is addressed in 4121be1: ProviderAuthV4 with StaticCredentialsProvider signs both AK and STS requests, preserves the security token, and resolves/validates the region before publication. I also rechecked conditional conflicts, uncertain PUT results, instance-scoped REST FileIO caching and session cleanup; no new blocking issue found.
Validation: 21 protocol tests and 28 REST/legacy tests passed. An independent simulated-transport probe passed 28 authentication/error-path scenarios and independently verified the V4 HMAC of 38 signed requests. These checks used the real oss2 SDK, without a live OSS bucket. The documented versioned/query-denied legacy fallback still lacks concurrent-commit protection, so the real atomic-write guarantee remains limited to confirmed unversioned buckets and cooperating conditional writers.
|
cc @sundapeng @timmyyao to take a review. |
|
Fix looks right.
|
Purpose
Fixes #9714.
Concurrent OSS writers can overwrite the same snapshot and lose committed data. Add
OssFileIOwith conditional PUTs (x-oss-forbid-overwrite=true), following Java #8228, while reusing existing PyArrow/Jindo file operations. Route OSS atomic writes through it and support Java-aligned SSE options via the optionaloss2dependency. Scope REST FileIO caches to each instance to isolate bucket and catalog settings.Conditional creation requires a bucket that has never enabled versioning. Versioned buckets or denied versioning queries fall back to legacy writes with a warning; that fallback does not protect concurrent commits.
Tests