felixhzhu commented on issue #4237:
URL: https://github.com/apache/amoro/issues/4237#issuecomment-4609852455
> 感谢您提供的详细信息。在确认这一点之前，我想先确定实际的触发条件，因为只有当对**不带斜杠**的键执行 HEAD 操作并返回 200
时，才会执行到第 95 行。
>
> `directoryPath`源自`TableFileUtil.getParent()`= `new
Path(path).getParent().toString()`，它**没有尾部斜杠**（例如`.../op_time_hour=12`）。对于，`S3FileIO`→指向该键，如果返回404
错误则返回。如果返回 404
错误，则直接返回，而不会执行抛出异常的操作。`exists()``BaseS3File.exists()``headObject(key =
uri().key())``false``TableFileUtil:90`
>
> 这削弱了扁平化 S3 上的三种机制（AWS/MinIO/Ceph）：
>
> * 目录标记带有斜杠_后缀_（`.../op_time_hour=12/`），因此对没有斜杠的键执行 HEAD 操作会得到不同的键 →
404。这些存储也不会在 PUT 操作时自动创建标记。
> * `HeadObject`这是一个精确键操作——非空前缀或列表式重写并不能使其返回 200。您有具体的网关实现可以做到这一点吗？
>
> 我还运行了 AWS 的“创建文件夹”脚本，但无法重现该问题：控制台输出`.../<partition>/`（带斜杠），但更简洁的 HEAD
请求`.../<partition>`（不带斜杠）返回 404。因此，“在每个对象存储表上 100% 成功”与代码在扁平 S3 上的执行情况不符。
>
> 如果对不带斜杠的目录键执行 HEAD 请求返回 200，则表明使用的是分层命名空间存储桶，而不是普通的扁平 S3
存储桶——能否确认一下存储桶的类型/模式？为了便于复现，请提供一个出现故障的分区目录，并提供原始`aws s3api head-object --key
<slash-less-key>`响应（200 或 404）以及完整`io-impl`的存储配置信息。
You're right, and I want to retract the over-reach before anything else.
### Where I was wrong
"100% on every object-store table" was sloppy. What I can actually claim is
narrower:
> **100% reproducible on our production environment**, with the
`IllegalArgumentException: null` stack trace from
`AuthenticatedFileIOAdapter:98` via `TableFileUtil.deleteEmptyDirectory:95` —
i.e. a black-box symptom observation. **Not** "HEAD on a slash-less key returns
200 on every S3-compatible store."
Your code reading on strict S3 (`BaseS3File.exists()` →
`headObject(uri().key())` on the slash-less key → 404 → early return at line
90) is correct. On flat AWS S3 / MinIO / Ceph with no placeholder marker and no
hierarchical-namespace mode, line 95 is unreachable. I should not have asserted
otherwise without raw HEAD evidence.
### What I don't yet have
I haven't captured the raw `aws s3api head-object` response on a failing
partition dir on our side. Until I do, I'm not going to defend any specific
mechanism. The plausible candidates I'd want to rule in/out — none of which I'm
asserting as fact right now — are:
- **(H1)** A 0-byte placeholder at the slash-less key (left over by some
historical write tool / SDK / `aws s3api put-object --key path/to/dir/`).
- **(H2)** Iceberg `S3InputFile.exists()` 1.6.1 falling through to a
non-pure-HEAD path under some retry/transient condition.
- **(H3)** **Bucket mode**: this is the one I think is most likely given
your line of reasoning. Our prod runs against Tencent Cloud COS via the
S3-compatible API, and COS does offer a hierarchical-namespace
("directory-flavoured") bucket mode. I need to verify whether the affected
bucket has it enabled — I don't want to guess.
### What I'll attach on the next reproduction
Exactly your asks, plus a few I think are useful:
- one specific failing partition path (anonymised);
- raw response of `aws s3api head-object --key <slash-less-key>` (status
code + body);
- raw response of `aws s3api head-object --key <slash-suffixed-key>`
(control);
- `aws s3api list-objects-v2 --prefix <slash-less-key>` output (to see
whether a placeholder marker actually exists);
- the table's `io-impl` and full `s3.*` / `write.object-storage.*`
properties;
- AMS DEBUG-level log lines containing `directoryPath` from the failing run;
- explicit confirmation of the bucket mode (regular S3-compatible vs. COS
directory-mode / hierarchical-namespace).
### A repro that doesn't depend on the S3 mechanism
While I get those artefacts, I'd like to separate the question "does this PR
fix a real contract violation" from "what exactly makes `exists()` return true
on our COS bucket". The first one I can answer deterministically without any
object store at all, because the bug is a contract violation between
`RollingFileCleaner` / `TableFileUtil.deleteEmptyDirectory` and
`AuthenticatedFileIO`, not an S3 quirk:
```java
@Test
void deleteEmptyDirectory_throwsOnPureObjectStoreFileIO() {
AuthenticatedFileIO io = mock(AuthenticatedFileIO.class);
when(io.exists(anyString())).thenReturn(true); // any path
"exists"
when(io.supportFileSystemOperations()).thenReturn(false); // pure
object-store contract
when(io.asFileSystemIO()).thenCallRealMethod(); // hits
Preconditions.checkArgument
assertThatThrownBy(() ->
TableFileUtil.deleteEmptyDirectory(io, "s3://bucket/db/t/data/p=1",
Set.of()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(null);
}
@Test
void deleteEmptyDirectory_isNoOpAfterFix() {
AuthenticatedFileIO io = mock(AuthenticatedFileIO.class);
when(io.exists(anyString())).thenReturn(true);
when(io.supportFileSystemOperations()).thenReturn(false);
TableFileUtil.deleteEmptyDirectory(io, "s3://bucket/db/t/data/p=1",
Set.of());
verify(io, never()).asFileSystemIO();
}
```
The two preconditions of the repro (`exists() == true` +
`supportFileSystemOperations() == false`) are **exactly the contract a pure
object-store `FileIO` advertises**. Anything that can satisfy the first half —
by any mechanism, HEAD/LIST/marker/hierarchical bucket — trips line 95. Happy
to push these as part of this PR; they run in milliseconds, no Testcontainers,
no S3.
### Why I still think the guard belongs at the entry point
Even granting your reading on strict flat S3, the same
`deleteEmptyDirectory` body calls `asFileSystemIO()` three more times after the
line-90 `exists()` guard:
```java
if (!io.exists(directoryPath)) return; // line
90
String parent = new Path(directoryPath).getParent().toString();
if (!io.asFileSystemIO().isDirectory(directoryPath) // line
95
|| exclude.contains(directoryPath) || exclude.contains(parent)) return;
...
if (io.asFileSystemIO().isEmptyDirectory(directoryPath)) { // line
100
io.asFileSystemIO().deletePrefix(directoryPath); // line
101
deleteEmptyDirectory(io, parent, exclude);
}
```
For the pre-fix code to be safe, every caller in every deployment in every
bucket configuration would have to guarantee `exists()` returns false for every
`directoryPath` the cleaner walks. The same author has already used the
`supportFileSystemOperations()` guard at `BasicTableTrashManager.java:104` and
`IcebergTableMaintainer.java:378`; the new `RollingFileCleaner` path is the
only place that broke the convention. Fixing the entry point re-aligns it.
### Proposal
1. I'll push the two unit tests above to this PR shortly so you can
reproduce the contract violation deterministically without any S3 dependency.
2. I'll follow up with the production HEAD artefacts (your full list above)
once we re-reproduce on the COS bucket, and post them as a comment here so we
can settle whether (H1)/(H2)/(H3) is the actual mechanism.
3. Until that lands, please treat the mechanism question as **open on my
side**, and the fix's justification as resting on (a) the deterministic
contract repro and (b) the three downstream `asFileSystemIO()` calls at lines
95/100/101 — not on my earlier "100% on every object-store table" wording,
which I'm withdrawing.
Does that work as a path forward?
--
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]
