felixhzhu commented on issue #4237:
URL: https://github.com/apache/amoro/issues/4237#issuecomment-4590145958
> Thanks for the report! The root cause checks out — for object-store
FileIOs the single-arg `checkArgument(boolean)` in `asFileSystemIO()` does
throw `IllegalArgumentException: null`.
>
> **question:** I can't quite square the "reproduces 100%" claim with the
`io.exists()` guard right before the throwing line:
>
> if (!io.exists(directoryPath)) return; // TableFileUtil:90
> ... io.asFileSystemIO() ... // :95 — throws
> For `S3FileIO`, `exists()` is a HEAD on the exact key, and `directoryPath`
is a slash-less prefix (`s3://.../data`) → 404 → `false` → returns at line 90,
never reaching 95. Verified on our own S3FileIO-backed catalog: a HEAD on the
slash-less prefix key returns 404, and I couldn't reproduce the failure there.
>
> Could you share:
>
> 1. the exact `io-impl` + Iceberg version,
> 2. which object storage you're on (AWS S3, Ceph RGW, MinIO, etc.), and
> 3. whether a HEAD on the slash-less prefix key returns **200** in your env?
>
> Reaching line 95 seems to need `exists()==true` for that prefix, which
looks storage/environment-specific rather than universal.
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]
