Repository navigation
Conversation
Without fsync, rename can publish empty snapshot/schema files after a crash on delayed-allocation filesystems (apache#10739). Flush and fsync file data in try_to_write_atomic, output streams, and format writers; fsync the parent directory after rename/create when the platform allows it.
Narrow durability to try_to_write_atomic and new_output_stream (format writers reuse the stream). File fsync must succeed before rename/close returns; fsync failure aborts publish. Tests assert ordering, API compat, failure paths, and parquet-via-stream; local catalog commit exercises the real write path.
Remove dead patch/repatch code from the fsync failure test. Keep ordering assertions, simple first-fsync failure injection, stream API checks, and parquet-via-stream coverage.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
The power-loss scenario in #10739 has clear end-to-end value. The new fsync, local/resolving IO and shared-shredding suites pass locally (76 cases, 31 subtests). Additional real ORC/Avro/ROW/BLOB/incremental Parquet callers round-trip and propagate injected file-sync errors. Two real catalog fault-injection cases below expose error-handling gaps at the durability boundary. No physical power-loss test was performed.
The project-configured Flake8 command also currently fails with F841 at local_file_io_fsync_test.py:168 (unused real_fsync); this revision has red Python CI jobs.
| return | ||
| try: | ||
| os.fsync(fd) | ||
| except OSError: |
There was a problem hiding this comment.
[P2] Propagate real storage failures from directory synchronization
This catch also swallows EIO/ENOSPC, although the best-effort exception is described for platforms that do not support directory fsync. With a real filesystem CatalogFactory table, I prepared one row and injected OSError(errno.EIO) only when os.fstat(fd) identified a directory. committer.commit(messages) returned normally and published snapshot 1 after ignoring six directory-sync failures; five regular-file syncs succeeded. A successful file fsync does not establish persistence of its new directory entry or of the snapshot rename, so this still acknowledges a commit when the requested persistence step failed. Please ignore only the explicit unsupported-platform cases and preserve genuine errors/uncertain-publication handling before reporting success.
| _fsync_file_obj(f) | ||
| except OSError: | ||
| # Do not rename: publishing would claim a durable snapshot/schema. | ||
| return False |
There was a problem hiding this comment.
[P2] Keep file-sync failures distinct from atomic-create contention
Returning False here feeds the unbounded retry loops in SchemaManager.create_table and SchemaManager.commit_changes, which interpret False as a schema publication race. I reproduced a real catalog create_table with persistent EIO on regular-file fsync: it repeatedly rewrites and retries schema-0 without exposing the storage error; the test had to stop at the fourth attempt. A control that propagates this OSError exits on the first attempt with the EIO cause. Please propagate the storage failure (while retaining temporary-file cleanup), or give the schema retry path a bounded error policy, instead of converting it to the contention sentinel.
Purpose
Fixes #10739. Python
LocalFileIOdid not sync data or temporary snapshot/schema files. On a delayed-allocation filesystem, a rename can publish a snapshot filename before its contents reach disk; after a power loss the latest snapshot may be empty and table reads fail.This change hardens the local write paths:
try_to_write_atomic: flush+fsync the temp file before rename; on fsync failure returnFalseand do not publish; after successful rename, best-effort fsync the parent directorynew_output_stream: close flushes+fsyncs file data first; fsync failure is raised after the fd is closed; parent dir fsync only after successful file fsyncnew_output_streamso they share one file-sync pathDirectory fsync is best effort where a platform does not support it. No physical power-loss test was run on this host.
Tests
local_file_io_fsync_test.py: 5 passed; checks sync order, fsync failure without publication, stream close behavior, and Parquet writing.file_io_test.pyandresolving_file_io_test.py: 56 passed. ORC-focused shared-shredding tests: 6 passed.CatalogFactorywarehouse commit and read: 2 rows returned; 13 fsync calls during the second commit.git diff --check: passed. One broader shared-shredding test could not run becausezstandardis absent on this host. The full Python CI matrix was not run locally.AI assistance was used for investigation, implementation, tests, review, and this description. No separate human code review is claimed.