JingsongLi commented on code in PR #10853:
URL: https://github.com/apache/paimon/pull/10853#discussion_r4407962552
##########
paimon-python/pypaimon/filesystem/local_file_io.py:
##########
@@ -262,7 +335,14 @@ def try_to_write_atomic(self, path: str, content: str) ->
bool:
try:
with open(temp_path, 'w', encoding='utf-8') as f:
f.write(content)
+ try:
+ _fsync_file_obj(f)
+ except OSError:
+ # Do not rename: publishing would claim a durable
snapshot/schema.
+ return False
Review 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.
##########
paimon-python/pypaimon/filesystem/local_file_io.py:
##########
@@ -33,6 +33,77 @@
from pypaimon.schema.data_types import PyarrowFieldParser
+def _fsync_file_obj(file_obj) -> None:
+ """Flush and fsync an open file object so its data reaches durable storage.
+
+ Raises OSError if the data cannot be synced; callers must not publish the
+ path as durable when this fails.
+ """
+ file_obj.flush()
+ os.fsync(file_obj.fileno())
+
+
+def _fsync_directory(dir_path: Path) -> None:
+ """Best-effort fsync of a directory so renames are ordered on disk.
+
+ Some platforms reject directory fsync; failures are ignored there. File
data
+ durability still depends on a successful prior file fsync.
+ """
+ try:
+ fd = os.open(str(dir_path), os.O_RDONLY)
+ except OSError:
+ return
+ try:
+ os.fsync(fd)
+ except OSError:
Review 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.
--
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]
