-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[python] Use conditional OSS puts for atomic metadata writes - #9715
[python] Use conditional OSS puts for atomic metadata writes #9715wangzhigang1999 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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
JingsongLi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
JingsongLi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
JingsongLi
commented
Sep 11, 2026
cc @sundapeng @timmyyao to take a review.
sundapeng
commented
Sep 11, 2026
Fix looks right.
- The new tests are well built but skipped in CI —
oss2isn't indev/requirements.txtor the workflow's install step. GetBucketVersioningruns on every call; caching the status on the instance would drop a request per write and make the fallback warning fire once instead of every time.
1ca7b27 to
6272192
Compare
wangzhigang1999
commented
Sep 12, 2026
Thanks @sundapeng! Updated in 6272192:
- Added
oss2to the test dependencies and CI install step. - Cached the versioning decision per FileIO, assuming stable bucket configuration. We accept duplicate first queries to avoid a lock and additional serialization hooks; the cache uses inherited pickle support.
All 21 OSS protocol tests passed locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires approval from a professional.
Uh oh!
There was an error while loading. Please reload this page.
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