[fix](iceberg) Harden external-write lifecycle, OCC, memory, and file ownership - #66348
Merged
Conversation
Gabriel39
requested review from
924060929,
englefly,
gavinchou,
liaoxin01,
luwei16,
morningman,
morrySnow and
starocean999
as code owners
August 1, 2026 08:16
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes: the static review found nine actionable issues (six P1, three P2), including three paths that can delete or reference missing Iceberg data, a concurrent use-after-free, an aggregate Thrift limit bypass, pathological spill reservation, and two incomplete object-storage cleanup paths.
Critical checkpoint conclusions:
- Goal and proof: The PR partially hardens write/cleanup/OCC behavior, and the overwrite and external-insert lifecycle changes are credible, but the nine inline findings prevent the stated lifecycle and cleanup goals from being met. Added tests prove only narrow single-state, AWS-abort, permutation, and executor cases.
- Scope and clarity: The edits are generally focused, but the new destructive action reimplements Iceberg safety policy incompletely, and the provider-neutral abort surface is implemented with AWS-specific state.
- Concurrency: The async writer publishes active-writer snapshots while a separately scheduled revocation task reads them. Per-writer
_sorter_mutexlocking is consistent and no deadlock was found, but it cannot protect the snapshot container because the range-for drops its temporary owner before iteration (MAIN-BE-003). - Lifecycle: Sort-writer close/cancel/deferred cleanup and external commit/listener ordering were traced end to end and otherwise preserve error ownership. S3/Azure abort lifecycle is incomplete: a rejected abort is never retried, and Azure reports successful abandonment without provider cleanup (MAIN-IO-001/002).
- Configuration: No new configuration was added. Existing
thrift_max_message_sizeand S3 limiter settings are read dynamically, but their consumers do not enforce the correct aggregate/cleanup contracts. - Compatibility: No storage-format or wire-field change was introduced. Default virtual methods preserve source-level parallel implementations, but that default is exactly why Azure silently lacks the new cleanup behavior.
- Parallel paths: Static/dynamic/branch/empty overwrites, data/delete writers, sync/async/destructor close, S3/Azure providers, and all insert executors were checked. The accepted findings identify the paths where parity is missing.
- Conditional checks: Existing overwrite conflict conditions are sound. The orphan action is missing the required
gc.enabledfence, safe retention interval, canonical URI identity handling, and version-hint reachability. - Test coverage: Coverage is not comprehensive. Missing cases include multi-task aggregate reports, concurrent snapshot publication/traversal, many-partition reserve accounting, hard-limit abort rejection, Azure staged-block abort, and execution-level orphan deletion tests for GC, retention, URI aliases, and the version hint.
- Test results: The new assertions are logically consistent within their narrow scopes; no result files changed. Builds and tests were intentionally not run because the authoritative review prompt forbids them, so this conclusion is static-only.
- Observability: Existing timers/logs cover writer/spill/commit paths, and abort failures are warned. No additional standalone observability defect was found, but logging does not substitute for retained cleanup ownership or retry.
- Transaction and persistence: No Doris EditLog state is added. Iceberg overwrite OCC anchors for main/branch/empty/static/dynamic cases are sound, and the durable external commit marker precedes refresh/listener suppression. The orphan action can nevertheless race a concurrent external commit (MAIN-FE-004).
- Data writes and atomicity: The aggregate-report failure can strand uncommitted objects; unsafe orphan deletion can corrupt shared, aliased, or concurrent writes; and multipart cleanup can leak provider state. These are blocking write-lifecycle defects.
- FE/BE propagation: No new FE-BE variable or Thrift field is introduced. Existing commit metadata is propagated on every producer path, but its budget is enforced per task rather than on the final aggregated RPC (MAIN-BE-001).
- Performance: Compact permutation dispatch is semantically sound, and bounded snapshot-copy cost is not independently material. Summing one full-batch reserve estimate per active partition can request roughly 128 blocks and repeatedly spill tiny partitions (MAIN-BE-002).
- Other issues: Error precedence, close idempotence, spill cleanup, overwrite validation, permission checks, location containment, and post-commit listener behavior produced no additional substantiated defects.
User focus: review_focus.txt contained no additional focus guidance, so the whole PR was reviewed without a narrower focus.
Review status: complete and converged in round 3. All final full-scope and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS for this exact comment set. Static review only; no builds or tests were run.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29107 ms |
Contributor
TPC-DS: Total hot run time: 169793 ms |
Contributor
ClickBench: Total hot run time: 23.92 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Aug 1, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: Parallel Iceberg sink tasks could exceed the shared report limit, partition writers could over-reserve memory or outlive a temporary snapshot, failed multipart uploads could skip mandatory cleanup, and orphan-file deletion lacked Iceberg safety fences. Share the report budget, retain writer snapshots, bound reservations, make abort cleanup provider-aware, and enforce safe canonical orphan detection. ### Release note Improve Iceberg write failure cleanup and orphan-file deletion safety. ### Check List (For Author) - Test: Unit Test - BE ASAN focused tests with Azure support enabled - Full fe-connector-iceberg unit test suite - Behavior changed: Yes, unsafe orphan cutoffs and GC-disabled deletion are rejected, and failed multipart uploads are cleaned up reliably. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two distinct issues remain after the current fixes: Azure abort can delete a valid pre-existing Put Blob object, and orphan-file reachability repeatedly scans inherited manifests with quadratic cost. I did not duplicate the existing review threads.
Critical checkpoint conclusions:
- Goal and proof: The PR broadly hardens Iceberg write/cleanup lifecycle, OCC, reporting limits, and orphan cleanup, but the Azure data-preservation goal is not yet met. The added tests cover nominal cleanup and OCC paths, not the two cases below.
- Scope and clarity: The changes are cohesive for the stated lifecycle work. The orphan action's hand-built reachability scan is the one area that should reuse Iceberg's unique-manifest approach.
- Concurrency: Async writer publication is protected by retained shared snapshots and per-sorter locking; the shared report budget has a consistent lock order. Azure's committed-block test is not a valid ownership fence for an existing object.
- Lifecycle: FE before-execution cleanup and post-commit error boundaries are sound, as are the reviewed nested writer close paths. Azure abort still destroys state it does not own.
- Configuration: No new configuration behavior requires dynamic-reload handling.
- Compatibility: No FE-BE or storage-format incompatibility was found. The changed Azure block-ID format has a bounded retry impact and did not survive as a separate finding.
- Parallel paths: S3, Azure, single-shot, multipart, committer, overwrite, branch, static/dynamic overwrite, and delete/merge paths were traced; the surviving provider-specific issue is inline.
- Conditional checks: GC, retention, URI containment, identity ambiguity, and overwrite validation checks are fail-closed.
CommittedBlocks.empty()is the invalid special case. - Test coverage: Unit coverage is substantial but misses preservation of an existing Put Blob target and an append-heavy multi-snapshot reachability scan. No end-to-end result covers either.
- Test results: No tests or builds were run because the authoritative review task is static-only; no generated result files were changed.
- Observability: Existing status/log paths are adequate for the reviewed failures; no additional metric was required for a correctness conclusion.
- Transactions and persistence: Iceberg overwrite OCC, including empty-table and branch anchors, is sound. No EditLog or master-failover persistence change is involved.
- Data writes: Commit/report cleanup ownership is sound outside the Azure abort branch; that branch can lose previously committed data and blocks approval.
- FE-BE variables: No new cross-version transmitted variable or missing propagation path was found.
- Performance: Per-snapshot
planFiles()causes the P2 quadratic manifest-read issue below; no other distinct hot-path issue survived duplicate suppression. - Other issues: The sorter aggregate-reservation concern remains covered by the existing thread, and cleanup admission/terminal-state concerns remain covered by the existing Azure/rate-limit threads, so they are not repeated here.
User focus: no additional user-provided focus was present; the full PR was reviewed.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29004 ms |
Contributor
TPC-DS: Total hot run time: 169560 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
previously approved these changes
Aug 10, 2026
yiguolei
previously approved these changes
Aug 10, 2026
Issue Number: None Related PR: apache#66348 Problem Summary: Azure block uploads are scoped to a blob rather than a provider multipart session, while the BE abort path duplicated transaction cleanup ownership that belongs to FE. Generate a local UUID for each Azure writer, encode the full UUID in every block ID, and remove leases and the generic BE abort interface. Competing same-key writers now stage disjoint blocks; once one commits, a later stale block list fails closed. None - Test: Unit Test - AzureObjStorageExtensionTest: 21 tests passed - Affected BE production and test objects compiled successfully, including Azure with USE_AZURE enabled - Repository clang-format 16 check and git diff --check passed - FE reactor Checkstyle passed with 0 violations - clang-tidy was attempted but blocked by repository/toolchain baseline errors in types.h and missing system stddef.h - Behavior changed: Yes, Azure multipart writers use local UUID block namespaces without leases, and BE no longer owns provider abort cleanup - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for three additional P1 correctness gaps found after a full-scope review, duplicate suppression against all 102 existing inline comments, and two convergence rounds.
New blockers:
- The transferred EOS reservation is released before Iceberg's real
close()finalization performs sorting and spill merging. - EOS sizing ignores the mandatory sort of buffered data below the generic append thresholds.
- An HMS lock timeout/RPC failure occurs before Hive's new self-rollback boundary, after the transaction manager has removed the rollback-capable owner.
Critical checkpoint conclusions:
- Goal and proof: The PR substantially hardens external-write OCC, report ownership, deferred upload publication, orphan deletion, and memory admission, but it does not fully accomplish the lifecycle/memory goal because the actual Iceberg finalization remains both unprotected and under-sized. The added tests exercise
finish()and empty-sorter fan-in arithmetic, not the productionclose()and buffered-sort paths; the Hive tests also omit lock-acquisition failure after valid upload reports. - Scope and clarity: The implementation is broad but mostly organized around the stated external-write lifecycle. The regression-framework MTMV lookup change is tangential, but its production schema and privilege path are consistent. No smaller local simplification removes the three blockers without repairing their lifecycle boundaries.
- Concurrency: The pipeline producer and fragment-manager writer coordinate through
_m, queue admission, and the moved reservation token; sorter write/reserve/revoke/close operations serialize under_sorter_mutex, and active-writer snapshots retain shared ownership. No new deadlock or lock-order defect survived review. The remaining BE failures are reservation lifetime/size errors rather than unsynchronized access. - Lifecycle: This is blocking.
AsyncResultWriterends the EOS reservation atfinish()although Iceberg finalizes inclose(). Separately, Hive loses the only cleanup-capable transaction owner before lock acquisition can fail. Error, cancellation, queue-drain, and non-OK close paths otherwise have concrete release/cleanup behavior. - Configuration: No new mutable BE/FE configuration item requires dynamic propagation. Existing query options and Iceberg/Hive table properties are consumed at the expected statement/runtime boundaries.
- Compatibility: New optional Thrift fields and capability checks generally fail closed for Iceberg and Azure during rolling upgrades. Remaining Azure same-key/legacy residual-ID concerns are already covered by existing review threads and were not duplicated.
- Parallel paths and conditions: Classic Coordinator and Nereids/Load report paths, standalone delete/table/MERGE sinks, final versus periodic reports, explicit-empty Iceberg pins, named/default branches, and static/dynamic/unpartitioned overwrite paths were traced. No additional divergent path or unexplained special condition survived duplicate suppression.
- Tests and results: The PR adds substantial BE and FE unit coverage plus regression-framework coverage, but the three accepted negative cases are missing. No build or test was run in this review environment because the task prompt explicitly prohibited builds and code modifications; this review therefore does not claim execution results.
- Observability: New failure paths generally carry query/table/lock context and memory/report counters remain available. Logging is adequate for diagnosis; no separate metrics blocker was found.
- Transactions and persistence: Iceberg statement pins and overwrite/RowDelta conflict validation retain an OCC fence, and the orphan action fails closed on ownership ambiguity. No new EditLog state is introduced. Hive's pre-lock cleanup ownership gap is the remaining transaction-lifecycle blocker.
- Data writes and crash behavior: Report ACK state is monotonic after ambiguous delivery, and failed Iceberg close paths clean local files. The accepted Hive issue can leave valid S3 multipart uploads open without metadata publication; the accepted BE issues can cross a hard memory limit during successful EOS finalization instead of reserving/spilling first.
- FE/BE variables: External commit vectors, report ACK capability, Azure multipart capability, and exact block IDs are wired through the changed Thrift and sender/receiver paths. No missing additional send site was found.
- Performance and other issues: Row-permutation dispatch removes the prior partition filter cost, and active-writer aggregation avoids serial-workspace overcounting. The remaining performance/correctness concern is the accepted forced-sort reservation gap. One cold-writer allocation hypothesis was explicitly dismissed after tracing
permute/insert_range_fromto exact-sizePODArray::resizerather than the 4 KiB push-growth floor.
User focus: no additional focus points were provided, so the entire PR was reviewed.
| _writer_status.update(st); | ||
| } | ||
| if (reservation_held_for_finish) { | ||
| thread_context()->thread_mem_tracker_mgr->shrink_reserved(); |
Contributor
There was a problem hiding this comment.
[P1] Keep the EOS reservation through the real Iceberg finalization. VIcebergTableWriter does not override finish() (the base implementation is a no-op); its partition writers run do_sort(), _do_spill(), and _combine_files_output() from close(), which is called only after this shrink_reserved(). Consequently the newly sized fan-in reservation is already gone when the merger allocates its cursors and blocks. Please release the token after close(st) on every path (ideally with an RAII guard), and make the async test observe the reservation inside close, not only an overridden finish.
| std::numeric_limits<size_t>::max())); | ||
| reserve.retained_growth_trigger_bytes = growth_trigger_bytes; | ||
| } | ||
| auto sort = new_rows > _buffered_block_size || new_block_bytes > _buffered_block_bytes; |
Contributor
There was a problem hiding this comment.
[P1] Include the forced EOS sort in this reservation. This method never uses eos and adds sort workspace only after the generic 4M-row/256 MiB append thresholds, but Iceberg close calls do_sort() for every nonempty sorter (or _do_spill(), which first sorts a pending run). A buffered sorter of tens of MiB below those thresholds can therefore reserve only the operator floor while sort_block keeps the source live and allocates the permutation plus a full permuted destination. The new fan-in test uses an empty sorter, so it misses both the no-spill and pending-run cases. Please size the actual EOS sort/serialization workspace and cover a buffered sorter below the normal thresholds.
| finishInsertTable(nameMapping); | ||
| // Classification can perform metastore reads, so close that interval before any file or HMS mutation. | ||
| validateWriteMetadataBeforePublication(); | ||
| try { |
Contributor
There was a problem hiding this comment.
[P1] Extend self-cleanup to lock acquisition failures. The rollback boundary still starts too late: PluginDrivenTransactionManager.commit() has already removed the transaction, so if acquireExclusiveTableLock times out or its RPC fails, this catch is never entered, the wrapper closes the connector, and the executor's later manager rollback finds no entry. Any valid S3 MPUs already fed into this transaction are then left open even though no Hive metadata was published. Please include lock acquisition in the connector's self-cleanup failure boundary (while releasing a lock only if it was acquired), and add a failure-injection test that verifies reported uploads are aborted on an HMS lock timeout/RPC failure.
Contributor
There was a problem hiding this comment.
Rechecked on current head 64eba05b2d: this P1 is still reachable. PluginDrivenTransactionManager.commit() removes the transaction before delegating, while HiveConnectorTransaction.commit() still acquires the HMS lock before entering the self-rollback boundary in commitWhileTableLocked(). Therefore an acquireExclusiveTableLock() timeout/RPC failure closes and deregisters the connector without aborting the valid deferred S3/Azure uploads already accepted from BE; the executor cannot recover them with a later manager rollback, and no Hive metadata is published. Please move lock acquisition under an outer failure boundary that invokes connector rollback when publication has not begun, and add a failure-injection test asserting all reported uploads are aborted when lock acquisition fails.
Contributor
TPC-H: Total hot run time: 28720 ms |
Contributor
TPC-DS: Total hot run time: 157981 ms |
Contributor
ClickBench: Total hot run time: 25.09 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29270 ms |
Contributor
TPC-DS: Total hot run time: 158006 ms |
Contributor
ClickBench: Total hot run time: 24.13 s |
924060929
approved these changes
Aug 14, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
yiguolei
approved these changes
Aug 14, 2026
Contributor
|
skip check_coverage |
gavinchou
reviewed
Aug 14, 2026
| return ObjectStorageUploadResponse { | ||
| .resp = resp, | ||
| // Hive defers completion to FE, so the exact staged ID must cross that boundary. | ||
| .etag = block_id, |
gavinchou
reviewed
Aug 14, 2026
| uint8_t buf[4]; | ||
| doris::encode_fixed32_le(buf, static_cast<uint32_t>(part_num)); | ||
| return Aws::Utils::HashingUtils::Base64Encode({buf, sizeof(buf)}); | ||
| std::string encode_azure_block_id(std::string_view upload_id, int part_num) { |
gavinchou
reviewed
Aug 14, 2026
| int part_num) { | ||
| DCHECK(opts.upload_id.has_value()); | ||
| auto client = _client->GetBlockBlobClient(opts.key); | ||
| std::string block_id = azure_multipart_block_id(*opts.upload_id, part_num); |
Contributor
There was a problem hiding this comment.
add some comment on "block_id" an "part_num"
gavinchou
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Iceberg and Hive external writes had correctness and resource-safety gaps across statement lifecycle, optimistic concurrency, asynchronous memory ownership, multipart publication, final-report ownership transfer, and orphan cleanup.
This PR:
Release note
Harden Iceberg and Hive external-write lifecycle, OCC conflict detection, memory admission, multipart publication, final-report ownership transfer, and orphan-file safety.
Testing
HiveConnectorTransactionTest: 17 passed, including validation/classification self-rollback and malformed-record isolation.
Iceberg connector focused suites: 295 passed after rebasing onto the latest Iceberg V3 changes.
Azure filesystem extension suite: 21 passed.
FE report acknowledgement, task-state, sink-binding, and insert-executor focused suites: 30 passed.
Earlier BE ASAN focused suites covered final-only report ownership, standalone DELETE ACK gating, active writer snapshots, and upstream missing-partition-source validation; the latest UUID update compiled all affected BE production and test objects.
External-file report lifecycle tests cover successful Hive close, periodic metadata deferral, and exact final pending-upload identity without BE provider abort ownership.
Sorted-write admission and ownership verification: 29/29 ASAN tests passed, covering multi-partition row/byte bounds, cold writer dispatch, reservation overflow, async write/EOS transfer, terminal cleanup, real spill fan-in, real 128-partition selection.
All pre-existing branch commits remained patch-equivalent after rebasing onto master.
Header self-containment compile verification: all four affected ANN translation units, RuntimeState, and PipelineFragmentContext passed ASAN syntax-only compilation after adding the missing Thrift forward declaration.
All changed BE C/C++ files passed the repository clang-format v16 check; the latest 12-file update also passed clang-format v16 dry-run and
git diff --check.Latest review-gap verification: QeProcessorImplReportAckTest passed 7/7, IcebergRemoveOrphanFilesActionTest passed 13/13, and the ASAN RuntimeStateIcebergCommitDataTest suite passed 7/7. Mutation checks failed when the timestamp and ambiguous-ownership guards were removed, then passed after the fixes were restored.
The three affected BE files passed the repository clang-format 16 check; the final diff passed
git diff --checkand the FE reactor completed Checkstyle with 0 violations.FE Checkstyle completed with 0 violations.
Latest Azure UUID update: AzureObjStorageExtensionTest passed 21/21; Azure BE production and test objects compiled with USE_AZURE enabled; clang-format v16 and git diff --check passed. Clang-tidy remained blocked by repository/toolchain baseline errors in types.h and the missing system stddef.h.
Connector forbidden-import architecture gate passed.
Focused clang-tidy found and fixed the changed sorter warning; full translation-unit analysis remains blocked by repository-baseline unconditional static assertions in
be/src/util/jni-util.h.Regression framework MTMV job lookup: the targeted test failed with the old cross-MV status query and passed with the isolated jobs metadata query; all 4 framework tests passed, with Java and Groovy compilation successful.
Iceberg merge sink BE UT: reproduced 12/14 failures with a mock coordinator missing report-ACK capability; after modeling an ACK-capable coordinator, all 14 merge sink tests passed, and the separate no-ACK fail-closed test also passed (15/15 focused ASAN tests).
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)