xlb3055 commented on PR #10853:
URL: https://github.com/apache/paimon/pull/10853#issuecomment-6654366573
# Proposed review reply for apache/paimon#10853
Thank you for the detailed review and the failure-injection examples. I
addressed both findings and the Python lint failure in a follow-up commit.
1. **Directory fsync errors are no longer swallowed indiscriminately.**
`_fsync_directory` now ignores only errors that explicitly mean directory
fsync is unsupported (`EINVAL`, `ENOTSUP`, or `ENOSYS` during fsync; `ENOTSUP`
or `ENOSYS` while opening the directory, plus Windows directory-open `EACCES`).
Storage and permission failures such as `EIO`, `ENOSPC`, and ordinary `EACCES`
are propagated with their original errno and exception cause. The directory
descriptor is still closed in every path.
2. **File fsync failures are no longer reported as atomic-write contention.**
`try_to_write_atomic` now propagates the sync failure instead of
returning `False`, while its `finally` block still removes the temporary file.
A normal rename race continues to return `False`, preserving the existing
contention behavior. This prevents `SchemaManager` from treating persistent
storage failure as a schema conflict and retrying without a bound.
3. **Known durability failures after publication are not acknowledged by the
commit retry path.**
I added a small `FileSyncError` subtype of `OSError` and let
`FileStoreCommit` re-raise it before the existing broad uncertain-commit retry
handler. This boundary is needed because a snapshot rename can already be
visible when the parent-directory fsync fails: retrying may find the visible
snapshot and incorrectly acknowledge the commit as a duplicate success. The
published snapshot and referenced files are preserved because their visibility
makes deletion unsafe, but the original operation still reports the durability
failure. Other commit exceptions keep the existing retry and uncertain-commit
semantics.
4. **Regression coverage now exercises the catalog paths.**
The tests inject file and directory sync failures through real catalog
schema creation and snapshot commit flows. They verify one-attempt propagation
for schema file `EIO`, snapshot-directory failure after rename without success
acknowledgement or file deletion, cleanup before publication, successful
commits where directory fsync is unsupported, and unchanged rename-contention
behavior. The new regression file has 16 passing tests; on the previous
implementation, 9 of them fail.
5. **Python CI lint failure is fixed.**
The unused `real_fsync` local that triggered `F841` was removed. The
repository Python license and Flake8 checks now pass.
Validation performed locally:
- `python -m pytest -q pypaimon/tests/local_file_io_fsync_test.py` — 16
passed
- Related Python tests on 3.10, 3.11, and 3.13 — 161 passed on each version
- Additional commit/cleanup/shared-shredding set — 81 passed, 18 skipped
because `pypaimon-rust` was not installed
- `bash dev/lint-python.sh -i license,flake8` — passed (`1253` license
headers, Flake8 clean)
- `git diff --check` — passed
No physical power-loss test was performed; the durability failures are
covered with deterministic syscall fault injection.
--
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]
