hubgeter opened a new pull request, #70502: URL: https://github.com/apache/doris/pull/70502
### Background The Iceberg `expire_snapshots` procedure commits snapshot expiration and then asks the SDK to clean up unreferenced files. Its result contains six counters for data files, position deletes, equality deletes, manifests, manifest lists and statistics files. ### What problem does this PR solve? Issue Number: None Related PR: #59979 (initial implementation), #69043 (file classification), #69703 (classification refresh) PR Type: Bugfix Problem Summary: The deletion callback incremented its success counter before calling `FileIO.deleteFile`. A failed attempt was therefore counted as a success, and retries inflated the result further: in a controlled manifest-list test, one failure followed by success reported 2 instead of 1, while four failed attempts reported 4 although the file remained present. Move the deletion call before the existing counter branches so only normal returns are counted, preserving the SDK's retry and cleanup behavior. Most of the diff adds failure-injection tests and creates the files whose successful deletion existing classification tests assert. ### Details - **Change map:** 3 changed production lines and 312 changed test lines; no generated files or documentation. The production change moves the existing deletion call; the tests cover failure handling and materialize existing classification fixtures. - **Frequency:** Triggered by deletion exceptions, including transient I/O errors and `NotFoundException`. Ordinary successful deletions keep their counts. - **Root cause:** All six counters were incremented before the operation they report. The SDK retries the callback and may suppress the final cleanup failure, so an incorrect count can be returned normally. - **Why existing tests missed it:** The success path did not inject deletion failures. Some classification fixtures recorded data or delete-file metadata without creating the corresponding in-memory files; premature counting made those failed deletions look successful. - **Fix:** Keep the existing classification, call `FileIO.deleteFile`, and increment the corresponding `AtomicLong` only after normal return. No catch, rollback, existence request or path-deduplication state is added. Counts represent normally completed FileIO calls, not a physical-object audit. - **Other paths and branches:** `remove_orphan_files` already counts after deletion. The current branch-4.2 legacy FE implementation retains the same ordering issue and needs a semantic backport to its own source and test paths; this PR targets master. ### Testing Locally ran the standard `run-fe-ut.sh --run` entry point with Maven scope restricted to `:fe-connector-iceberg -am`, build cache disabled and Checkstyle enabled. The selection was all `org.apache.doris.connector.iceberg.action.*Test` classes plus `IcebergProcedureOpsTest` and `IcebergProcedureOpsKerberosIoTest`. - 153 tests passed, 2 existing disabled tests skipped, 0 failures or errors. - All 21 new deletion tests passed. They exercise real SDK snapshot expiration with controlled FileIO failures across all six file kinds, normal returns, retries, permanent failures, concurrent mixed outcomes, NotFound and a lost deletion response. A latch ensures concurrent callbacks overlap. - Before the production fix, 15 of the 21 new cases failed their count assertions, with no execution errors. The mixed concurrent case returned 18 instead of 7. - Existing classification tests retain their expected counts after their target files are materialized. - Iceberg module Checkstyle: zero violations. No full FE/BE build, cluster SQL regression, real object-store failure injection or remote CI result is claimed. Metadata-commit failure was not dynamically injected. ### Release note Iceberg `expire_snapshots` excludes failed deletion attempts from its deleted-file counts. ### Check List (For Author) - Behavior changed: Yes. Deleted-file counts exclude calls that throw; file selection, retry policy and the six-column schema are preserved. - Does this need documentation: No. - Pre-submission status: Partially verified: focused local unit tests and Checkstyle passed; full build, cluster regression and remote CI have not been verified. ### AI Disclosure - Tool: Codex. - Generated: Code, tests and this description. - Human review: Pending. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
