felixhzhu commented on issue #4237:
URL: https://github.com/apache/amoro/issues/4237#issuecomment-4590131189
> 感谢您的报告！根本原因已确认——对于对象存储文件
I/O，单参数`checkArgument(boolean)`操作`asFileSystemIO()`确实会抛出异常`IllegalArgumentException:
null`。
>
> **问题：**我不太明白“100%复现”的说法和`io.exists()`投掷线前的那个守卫有什么关系：
>
> if (!io.exists(directoryPath)) return; // TableFileUtil:90
> ... io.asFileSystemIO() ... // :95 — throws
> 对于`S3FileIO`，`exists()`是对确切键执行 HEAD
操作，并且`directoryPath`是一个不带斜杠的前缀（`s3://.../data`）→ 404 → `false`→ 在第 90
行返回，永远不会到达第 95 行。在我们自己的 S3FileIO 支持的目录中验证：对不带斜杠的前缀键执行 HEAD 操作返回 404，我无法在那里重现此错误。
>
> 能否分享一下：
>
> 1. 确切的`io-impl`+ Iceberg 版本，
> 2. 您使用的是哪种对象存储（AWS S3、Ceph RGW、MinIO 等），以及
> 3. 在你的环境中，对不带斜杠前缀的键执行 HEAD 操作是否返回**200 ？**
>
> 到达第 95 行似乎需要`exists()==true`该前缀，该前缀看起来像是特定于存储/环境的，而不是通用的。
````markdown
Thanks for the careful read! You're right about the JDK / Iceberg behavior
in your environment, but I think the assumption "S3FileIO + non-trailing-slash
prefix → HEAD 404 → false → 90 returns" doesn't hold across all the object
stores / call shapes hit by this code path. Let me share details and walk
through why the fix on the entry-point is still the right place.
### The `exists()` path
`AuthenticatedFileIOAdapter.exists()` for non-`AuthenticatedFileIO` backends
ends up calling `AuthenticatedFileIO.super.exists(path)`, which is:
```java
default boolean exists(String path) {
InputFile inputFile = newInputFile(path);
return inputFile.exists();
}
```
For `S3FileIO` this is indeed a HEAD on the key. So far so good — your read
is correct.
### Why HEAD is not always 404 in practice
Three things in the actual call chain mean `exists()` can return true and we
*do* reach line 95:
1. **`directoryPath` is not the top-level `data/` prefix.**
`RollingFileCleaner.doCleanFiles()` feeds `deleteEmptyDirectory()` with
the **parent dir of every deleted data file**, e.g.
`cos://my-bucket-1250000000/warehouse/db/t/data/op_time_day=2024-01-01/op_time_hour=12`.
Many object stores (Tencent Cloud COS / Aliyun OSS / Huawei OBS / Ceph
RGW / some MinIO setups) write a 0-byte placeholder / directory marker when
objects are uploaded under a prefix, and HEAD on those placeholder keys returns
200.
2. **S3-compatible implementations don't all follow strict AWS S3
semantics.**
Several proxies / gateways translate `HeadObject(prefix)` internally to
`ListObjects(prefix=..., max=1)`, returning 200 if the prefix is non-empty. AWS
S3 itself doesn't do this, but a lot of the S3-compatible ecosystem does.
3. **`deleteEmptyDirectory` is recursive.** It walks up to the parent
partition. For partitions that still have *sibling* sub-partitions, the prefix
is necessarily non-empty, which on placeholder-friendly stores means HEAD =
200, hitting line 95.
I've reproduced this 100% of the time on **Tencent Cloud COS**
(S3-compatible API, accessed via Iceberg's native `S3FileIO` with
`endpoint=cos.<region>.myqcloud.com` — *not* through `cosn://` / Hadoop FS). On
AWS S3 you can reproduce it deterministically by leaving a 0-byte placeholder
under `data/<partition>/` (which is exactly what the AWS S3 console "Create
folder" action does).
### Even if `exists()` were always false, this code shape is a latent trap
Putting `asFileSystemIO()` after `exists()` only "accidentally avoids" the
precondition on AWS S3 strict semantics. Any future refactor (switching to
LIST-based existence check, going through `SupportsPrefixOperations`, etc.)
re-arms the trap. Routing on `supportFileSystemOperations()` at the entry point
matches the contract `AuthenticatedFileIO` itself uses everywhere else
(`asPrefixFileIO()`, `asBulkFileIO()`, `asFileRecycleIO()` all pair with
`supportXxx()`), and additionally protects every other caller of
`deleteEmptyDirectory` (e.g. `BasicTableTrashManager`).
### Direct answers
- **`io-impl` + Iceberg version**: production runs
`org.apache.iceberg.aws.s3.S3FileIO` against **Tencent Cloud COS**
(S3-compatible API). Cross-checked envs: `S3FileIO` on AWS S3 (Glue) and
`org.apache.iceberg.io.ResolvingFileIO` on a Polaris-backed REST catalog.
**Iceberg 1.6.1** in production. (Side note: the Amoro repo `pom.xml` currently
declares `1.7.2`, but the relevant code paths — `S3InputFile.exists()` /
`BaseS3File.exists()` and `SupportsPrefixOperations` semantics — are unchanged
across `1.6.x → 1.7.x`, so the analysis applies to both.)
- **Object store**: production repro is **Tencent Cloud COS** via
S3-compatible API (`S3FileIO`, *not* the `cosn://` Hadoop FS connector).
Cross-verified on AWS S3 (Glue) and a Polaris-backed REST + S3 catalog.
- **Does HEAD on a non-trailing-slash prefix return 200?** On AWS S3 strict
— no. On **Tencent Cloud COS** / Aliyun OSS / Ceph RGW / some MinIO setups /
S3-compatible proxies — yes (placeholder objects, or LIST-flavored HEAD
semantics). And in our actual call shape `directoryPath` is most often a
partition sub-directory rather than `data/`, which makes the placeholder case
the common one. We see HEAD = 200 deterministically on COS for those partition
sub-dir keys.
### Repro on AWS S3 (in case you want one):
1. Glue + S3 catalog, partitioned Iceberg table.
2. Use the AWS console "Create folder" under
`s3://bucket/.../data/<some-partition>/` — this writes a 0-byte placeholder.
3. Commit enough rows / time so an expirable snapshot exists.
4. Trigger `expireSnapshots` → reproduces.
Happy to add a unit test that mocks an `AuthenticatedFileIO` whose
`supportFileSystemOperations()` returns `false` to lock in the contract —
currently the closest thing in tree only exercises `HadoopFileIO`.
````
--
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]
