XiaoHongbo-Hope commented on code in PR #9821:
URL: https://github.com/apache/paimon/pull/9821#discussion_r4013702413
##########
paimon-python/pypaimon/filesystem/pyarrow_file_io.py:
##########
@@ -465,6 +497,34 @@ def delete(self, path: str, recursive: bool = False) ->
bool:
self.filesystem.delete_file(path_str)
return True
+ def _delete_s3_compatible_directory(self, path_str: str) -> bool:
+ selector = pafs.FileSelector(
+ path_str, recursive=True, allow_not_found=True)
+ file_infos = self.filesystem.get_file_info(selector)
+ files = [
+ info.path for info in file_infos
+ if info.type == pafs.FileType.File
+ ]
+ if files:
+ with ThreadPoolExecutor(max_workers=min(16, len(files))) as
executor:
+ list(executor.map(self.filesystem.delete_file, files))
+ directories = sorted(
+ (info.path for info in file_infos
+ if info.type == pafs.FileType.Directory),
+ key=lambda item: item.count("/"),
+ reverse=True,
+ )
+ for directory in directories:
+ self._delete_s3_directory_marker(directory)
+ self._delete_s3_directory_marker(path_str)
+ return True
+
+ def _delete_s3_directory_marker(self, path_str: str):
+ try:
+ self.filesystem.delete_dir(path_str.rstrip("/"))
Review Comment:
> **Avoid re-entering `DeleteObjects` during directory-marker cleanup.**
>
> `S3FileSystem.delete_dir()` does more than remove the marker: it
recursively lists the directory and deletes any remaining children via
`DeleteObjects` before deleting the marker. See [the PyArrow 23
implementation](https://github.com/apache/arrow/blob/apache-arrow-23.0.0/cpp/src/arrow/filesystem/s3fs.cc#L3251).
>
> I reproduced this with a real PyArrow 23 client against a local endpoint
requiring `Content-MD5` for batch deletion: list `table/data/{a,b}`, delete
those files, and insert `table/data/late` before this call. The call sends
`POST ?delete` without `Content-MD5` and fails with `MissingContentMD5`,
leaving the new file and directory marker behind.
>
> The current workaround therefore avoids batch deletion only while the
directory remains unchanged. Could we ensure marker cleanup cannot re-enter
that incompatible API, and add a request-level regression test with an object
appearing between listing and marker cleanup? Mocking `delete_dir()` hides this
behavior.
Thanks, fixed
##########
paimon-python/pypaimon/filesystem/pyarrow_file_io.py:
##########
@@ -465,6 +497,34 @@ def delete(self, path: str, recursive: bool = False) ->
bool:
self.filesystem.delete_file(path_str)
return True
+ def _delete_s3_compatible_directory(self, path_str: str) -> bool:
+ selector = pafs.FileSelector(
+ path_str, recursive=True, allow_not_found=True)
+ file_infos = self.filesystem.get_file_info(selector)
+ files = [
+ info.path for info in file_infos
+ if info.type == pafs.FileType.File
+ ]
+ if files:
+ with ThreadPoolExecutor(max_workers=min(16, len(files))) as
executor:
+ list(executor.map(self.filesystem.delete_file, files))
Review Comment:
> **Tolerate objects that disappear after the directory listing.**
>
> PyArrow's `delete_file()` performs a HEAD request first and raises
`FileNotFoundError` if the object is already gone. Here that exception
propagates through `executor.map()` and skips the directory-marker cleanup.
>
> I reproduced this by removing one listed object before its HEAD request,
simulating another cleanup task deleting it. The PyArrow 23 fallback raised
`FileNotFoundError` and left a marker behind; the original batch-deletion path
with PyArrow 21 completed successfully for the same scenario.
>
> Please wrap individual deletions so that `FileNotFoundError` is treated as
successful deletion, while permission, transport, and other failures still
propagate. A regression test should remove an object after listing and verify
that cleanup completes.
Thanks, fixed
--
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]
