branch-4.1: [fix](metacache) release Iceberg FileIO and Hudi fsView on cache eviction #66914 - #66913
Open
924060929 wants to merge 34 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
924060929
changed the title
924060929
force-pushed
the
fix/cir-21497-iceberg-fileio-lifecycle-4.1
branch
from
August 19, 2026 02:54
e4fc329 to
34f4821
Compare
924060929
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
924060929
changed the title
924060929
changed the title
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
Contributor
FE UT Coverage ReportIncrement line coverage |
924060929
force-pushed
the
fix/cir-21497-iceberg-fileio-lifecycle-4.1
branch
from
August 20, 2026 08:40
1a252c3 to
eb1e148
Compare
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 resource-release patch currently contains seven P1 lifecycle/ownership defects and one P2 REST integration defect.
Critical checkpoints:
- Correctness and resource ownership: Hudi does not retain the exact acquired cache generation, permits acquire-after-close, and has no retirement path for uncached/suppressed loads. Iceberg closes escaped table generations while active and treats DLF's catalog-wide FileIO as table-owned.
- Concurrency and terminal paths: Hudi batch planning can leak its lease after partial submission, while the synchronous path can either leak before its try/finally or release while accepted tasks are still using the view.
- Catalog/API compatibility: the REST check targets RESTSessionCatalog even though Iceberg 1.10.1 returns RESTCatalog, so that branch is unreachable. Glue per-operations ownership itself is valid, but immediate cache-removal close is still unsafe for active consumers.
- Test coverage: the added Iceberg test bypasses the real ownership classifier and retains no active consumer. Coverage is still needed for DLF two-table identity, REST runtime types, refresh/invalidation with active generations, disabled/manual-load paths, Hudi eviction races, and all split-planning terminal outcomes.
- User focus: no additional focus was supplied; the complete ten-file change and its relevant consumers were reviewed.
- Review completeness: all candidates were independently verified and deduplicated. Three Round 2 convergence sweeps returned NO_NEW_VALUABLE_FINDINGS.
No builds or tests were run because the review instructions explicitly prohibited them.
924060929
force-pushed
the
fix/cir-21497-iceberg-fileio-lifecycle-4.1
branch
from
August 20, 2026 10:17
eb1e148 to
df0eb28
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes: two P1 correctness/lifecycle defects and one conditional P2 scalability regression remain after the current fixes.
Critical checkpoints:
- Correctness and freshness: disabling filesystem-view refresh allows a hot T1 view to plan a T2 query without newly committed base files or MOR log slices.
- Lifecycle and resource ownership: the view lease is acquired before the planner publishes a cleanup owner; failures before split generation or after batch work starts can skip deterministic stop/join/release.
- Concurrency and configuration scalability: the single Phaser creates a new 65,534-outstanding-task ceiling under a reachable executor and session configuration.
- Cache API and compatibility: no additional distinct issue survived review after tracing the wrapper handoff, invalidation races, disabled/manual modes, constructors/overloads, and all in-repo consumers. The existing MetaCacheEntry suppressed-load thread remains the duplicate fence.
- Terminal paths: ordinary rejection, interruption, stop, and worker-error accounting is balanced after this revision; no additional issue remains beyond the planner-handoff gap and existing threads.
- Test coverage: the changed tests cover local reference/removal behavior, but not hot T1/T2 freshness, planner-abort cleanup, or the Phaser boundary.
- User focus: no additional focus was supplied; the complete eight-file change was reviewed.
- Review completeness: three review rounds completed and the final normal and risk-focused sweeps all returned NO_NEW_VALUABLE_FINDINGS. Every candidate is accepted, dismissed with evidence, or duplicate-fenced.
No builds or tests were run because the review instructions explicitly prohibited them.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete; the review converged in Round 2.
Requesting changes for three P1 correctness regressions:
- Server-prepared Iceberg and batch-Hudi queries reuse a StatementContext permanently closed after COM_STMT_PREPARE, so COM_STMT_EXECUTE fails at resource registration.
- Arrow Flight closes the Hudi batch owner after GetFlightInfo, stopping the lazy split source before the deferred DoGet consumer finishes and potentially returning incomplete results.
- Disabling Iceberg auto-refresh lets continuously accessed entries remain pinned to stale externally committed snapshots.
Critical checkpoints covered: all 15 authoritative production/test paths and every diff hunk; Hudi exact-generation handoff, freshness, non-batch joins, batch accounting, rejection/cancellation/terminal paths, and Arrow lifetime; Iceberg cache/loader/lease arithmetic, snapshot freshness, catalog-specific FileIO ownership, direct/background consumers, and rewrite cleanup; MetaCacheEntry invalidation/manual-load/removal callback ordering; prepared/reset/close and ordinary/background StatementContext lifecycles. Existing live threads and replies were treated as duplicate fences; these three issues are distinct. The executor-rejection candidate was dismissed after verifying the serving executor throws into the Doris/Caffeine inline fallback and the checkpoint-only silent executor has no resource-bearing load path.
User focus: no additional focus was provided; the full PR review found no other non-duplicate issue. Per the review-runner constraint, this was a static review and no builds or tests were run.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: capped/incomplete at the three-round limit. Round 3 surfaced one additional accepted production owner (AutoCloseConnectContext for external analysis); per the review-runner contract I am submitting the verified findings now without claiming convergence.
Requesting changes for four new P1 lifecycle/configuration regressions:
- Master-side forwarded execution never closes the proxy-created
StatementContext, permanently pinning statement-scoped external resources. - Non-cache Iceberg property updates retire the runtime without invalidating its table cache, so new statements can keep selecting the old endpoint/warehouse/credential generation.
- Catalog retirement defers only
Catalog.close()while immediately shutting down the generation's pre-auth executor, so already-leased scans/transactions can fail or cross runtime generations. AutoCloseConnectContextonly clears/nulls the currentStatementContext; external statistics queries therefore discard registered Iceberg leases without closing them.
Existing-thread verification: production COM_STMT_EXECUTE still opens a generation that MysqlConnectProcessor.handleExecute() never closes, and Hudi statement cleanup still cannot cancel accepted queued producer/worker work before its uninterruptible terminal wait. Those mechanisms remain covered by their existing live threads, so I did not add duplicate inline comments.
Critical checkpoints: all 25 authoritative changed paths and every diff hunk were reviewed for correctness, lifecycle, concurrency, freshness, configuration, compatibility, performance, exception/rejection behavior, and production-shaped coverage. Cache reference accounting, current hot-entry refresh, catalog-specific FileIO classification, and the normal processor, task-processor, MTMV, dictionary, rewrite, and current Arrow close paths produced no further nonduplicate finding. The added tests do not cover the four production boundaries above.
User focus: no additional focus was provided; the full PR was reviewed. This was the required static review, so no builds or tests were run.
Contributor
FE UT Coverage ReportIncrement line coverage |
924060929
force-pushed
the
fix/cir-21497-iceberg-fileio-lifecycle-4.1
branch
from
August 21, 2026 07:24
0cb4ab6 to
f090dcc
Compare
Fence legacy catalog property candidates from concurrent initialization and serialize Arrow Flight result publication with connection teardown.
Issue Number: close apache#66913 Related PR: apache#66914 Problem Summary: Rebasing the Iceberg and Hudi handle leak fix onto branch-4.1 introduced the new external metadata cache memory-governance implementation. The old lifecycle APIs no longer compiled, and a textual merge would either lose exact-generation ownership or bypass the new weight, refresh, and snapshot-isolation contracts. This change integrates statement and asynchronous borrowers with the governed cache, retires replaced and removed Iceberg generations only after their final borrower, closes table-owned FileIO and catalog generations in the captured authentication scope, and preserves Hudi removal cleanup when a cache group closes. It also removes stale pre-governance API residue and updates lifecycle regression tests to exercise the governed refresh path. Fix Iceberg and Hudi metadata handle leaks during cache refresh, eviction, invalidation, and catalog reset. - Test: Unit Test and FE build - ./run-fe-ut.sh targeted Iceberg, Hudi, and StatementContext tests - ./build.sh --fe - Behavior changed: Yes (external metadata handles are closed after their exact generation is no longer borrowed) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: Closing a branch-4.1 external metadata cache could race asynchronous removal publication or cleanup after a worker had claimed a token, dropping dependency retirement callbacks. Hudi batch split cancellation was also only owned by StatementContext, so SplitAssignment cancellation could leave started filesystem tasks running without promptly requesting interruption. Give each removal token an exact single owner across publication, cleanup, and close; register the Hudi batch owner with SplitAssignment and make cancellation idempotently interrupt all tracked tasks while retaining the fs-view lease until terminal accounting completes. ### Release note None ### Check List (For Author) - Test: Unit Test - MetaCacheEntryTest, SplitAssignmentTest, HudiFsViewCacheValueTest, HudiBatchFsViewOwnerTest, HudiScanNodeTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: After an FE upgrade or restart loads an Iceberg or HMS catalog from a metadata checkpoint, Gson bypasses Java field initializers. The runtime-only Iceberg resource trackers and HMS runtime generation therefore remain null, causing Iceberg table loads to fail and leaving HMS lifecycle fencing unusable. Recreate these runtime-only objects in each catalog's gsonPostProcess hook after the persisted state is restored. ### Release note Fix Iceberg catalog queries and writes after FE metadata checkpoint recovery. ### Check List (For Author) - Test: Unit Test / Manual test - ExternalCatalogRuntimeStateTest - ./build.sh --fe - Docker upgrade and checkpoint restart with Iceberg REST SELECT and INSERT - Behavior changed: Yes, replayed Iceberg and HMS catalogs restore their runtime lifecycle state - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: SplitAssignment.stop rethrew an already-reported planning failure after releasing assignment resources, which could prevent split-source and sibling scan cleanup during query cancellation. DorisHiveCatalog also skipped its owned FileIO when Iceberg inherited close threw an unchecked reporter failure. Keep assignment stop no-throw and execute both catalog close legs while preserving the primary failure and suppressing later failures. ### Release note None ### Check List (For Author) - Test: Unit Test - SplitAssignmentTest, FileQueryScanNodeTest, DorisHiveCatalogTest, ExternalCatalogRuntimeStateTest - ./build.sh --fe - Behavior changed: Yes. Cleanup now completes after an existing planning or close failure. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: Hudi held its generation-owner monitor while synchronizing a remote filesystem view, so a stalled timeline sync could block catalog reset or ALTER cleanup. Iceberg background refreshes also leaked loaded table generations when mutation fencing or weighted admission prevented publication. Limit the Hudi monitor to exact lease ownership changes, and retire unpublished metadata refresh values through a dedicated callback that does not invalidate the retained Iceberg generation child caches. ### Release note None ### Check List (For Author) - Test: Unit Test - HudiFsViewCacheValueTest, MetaCacheEntryTest, IcebergExternalMetaCacheTest - ./build.sh --fe - Behavior changed: Yes. External metadata cleanup no longer waits for Hudi timeline I/O, and unpublished Iceberg refresh generations release their resources without invalidating the retained generation. - Does this need documentation: No
Issue Number: None Related PR: apache#66914 Problem Summary: Hudi incremental planning acquired and synchronized an unused shared filesystem view, while its task-lifecycle tests could race the terminal callback. Iceberg resource-owning weighted cache values could lose their cleanup identity through soft collection; snapshot projection and table-property DDL could also cross catalog generations. Hive catalog initialization could allocate FileIO before a later client-pool failure and leak it. Keep resource owners strongly reachable until bounded eviction, use one captured authenticator and generation-bound writable table, make partial catalog initialization failure-atomic, and avoid the unused Hudi view acquisition. None - Test: Unit Test - HudiBatchFsViewOwnerTest, HudiScanNodeTest, HudiExternalMetaCacheTest - DorisHiveCatalogTest, IcebergMetadataOpsValidationTest, IcebergExternalMetaCacheTest - MetaCacheEntryTest - Behavior changed: Yes. Iceberg and Hudi resource generations now preserve cleanup ownership across cache eviction, initialization failure, and catalog refresh races. - Does this need documentation: No
Issue Number: None Related PR: apache#66913 Problem Summary: Iceberg table, snapshot, schema, writable-table, and Hudi file-system-view handles could cross catalog generations or lose their close boundary during empty scans, refresh, property updates, cache replacement, and failure paths. Capture one coherent catalog generation, retain exact ownership through leases and cleanup tokens, reject mixed generations, and close resources exactly once after the final borrower releases them. Fix Iceberg and Hudi external metadata resource lifecycle leaks. - Test: Unit Test - HudiScanNodeTest - IcebergExternalMetaCacheTest - IcebergMetadataOpsValidationTest - Behavior changed: Yes. External metadata handles are retained and closed at their owning generation boundary. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66913 Problem Summary: Fix the FE Checkstyle field-separation violation introduced while rebasing the Iceberg lifecycle tests. This is a formatting-only repair and does not change Iceberg or Hudi behavior. ### Release note None ### Check List (For Author) - Test: No need to test (formatting-only change); full FE Checkstyle passed. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66913 Problem Summary: Iceberg Hadoop catalog generations did not own and close their catalog-wide FileIO, and historical partition projections joined current partition fields to historical schemas by mutable names. This change gives each Hadoop catalog generation idempotent FileIO ownership, matches partition columns by Iceberg field ID, keys cached projections by the current schema identity, and fixes the deterministic frozen-spec test fixture. ### Release note Fix Iceberg catalog FileIO retirement and historical partition-column projection across schema renames. ### Check List (For Author) - Test: Unit Test - DorisHadoopCatalogTest - IcebergUtilsTest - IcebergExternalMetaCacheTest - Behavior changed: Yes (Iceberg Hadoop catalog generations release their shared FileIO, and historical partition columns remain stable across renames) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66913 Problem Summary: Hudi synchronous listing cancellation could complete terminal accounting before publishing cancellation and return partial splits. Catalog-property replay could publish new properties before the runtime generation reset, and Hudi generation reads could observe that intermediate state. Iceberg frozen tables also dropped weak FileIOTracker keys while Doris independently closed SDK-owned REST, Glue, and S3 Tables FileIO. This change publishes cancellation before task callbacks, commits replay properties once through the synchronized runtime reset, reads the HMS generation under the same monitor, and retains SDK-tracked operations while leaving FileIO closure to each catalog tracker. ### Release note Fix Hudi cancellation, catalog replay generation fencing, and Iceberg FileIO tracker ownership. ### Check List (For Author) - Test: Unit Test - ExternalCatalogRuntimeStateTest - HudiBatchFsViewOwnerTest - CatalogMgrTest - IcebergTableCacheValueTest - IcebergExternalMetaCacheTest - Behavior changed: Yes (cancelled Hudi listing fails instead of returning partial splits, replay swaps properties with runtime state atomically, and catalog SDK trackers remain the sole per-table FileIO owners) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66913 Problem Summary: Iceberg catalog operations could outlive a catalog reset while their Catalog and FileIO generation was retired, and could combine an old catalog with new runtime properties or database name mappings. Retain the exact runtime generation for direct operations, freeze generation configuration, and resolve database identity under the catalog reset monitor for both dedicated Iceberg and HMS catalogs. ### Release note Fix Iceberg catalog operation lifetime during runtime reset. ### Check List (For Author) - Test: Unit Test - Iceberg lifecycle, metadata operation, cache, scan node, and runtime-state FE unit tests - ./build.sh --fe - Behavior changed: Yes (Iceberg catalog operations retain one consistent runtime generation until completion) - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#66912 Related PR: apache#66913 Problem Summary: Iceberg create-table tests constructed a second metadata-operations object outside the catalog-owned runtime and were correctly rejected by the exact-generation guard. Hudi scan planning could also resolve the current filesystem-view cache after capturing an older catalog runtime, run filesystem-view synchronization outside the captured authenticator, or deadlock catalog reset if a cold load and remote sync were kept under the cache lifecycle monitor. In addition, mixed-case Iceberg catalog type values were frozen without normalization and later rejected by lowercase-only database validation. Use the catalog-owned Iceberg metadata operations in tests and normalize the frozen Iceberg catalog type. Capture the exact Hudi fs-view cache generation and authenticator with the HMS runtime, load outside lifecycle monitors, atomically validate and pin the exact view, and run remote synchronization under the captured authenticator after the pin. A reset that wins during a cold load retires the unpublished view and cannot redirect the scan to a replacement generation. ### Release note Fix Iceberg and Hudi metadata resource handling across catalog reset and property changes. ### Check List (For Author) - Test: Unit Test - CreateIcebergTableTest - IcebergMetadataOpTest - HudiExternalMetaCacheTest - HudiFsViewCacheValueTest - HudiScanNodeTest - ./build.sh --fe - FE Checkstyle - Behavior changed: Yes. Hudi filesystem-view access remains bound to the captured catalog generation and authentication context; mixed-case Iceberg catalog types are normalized. - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#66913 Related PR: apache#66913 Problem Summary: Iceberg metadata mutations could release the writable catalog generation before branch, tag, schema, or partition commits completed, allowing a concurrent catalog reset to retire resources still used by the commit. Hudi native-reader schema resolution could similarly cross an HMS runtime reset and publish schema information from the retired generation. Hold the exact writable Iceberg table lease and its captured authenticator through every metadata commit, and revalidate the HMS runtime generation after Hudi schema resolution before publishing schema history or range descriptors. ### Release note Fix Iceberg and Hudi external catalog resource lifecycle races during metadata mutation and scan planning. ### Check List (For Author) - Test: Unit Test - IcebergMetadataOpsValidationTest - IcebergExternalTableBranchAndTagTest - HudiScanNodeTest - Full FE build - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: Iceberg table handles, snapshots, actions, transactions, and asynchronous rewrite tasks could outlive the cache or catalog generation that created their FileIO, authenticator, and planning executor. Hudi runtime capture could also invert the lifecycle-stripe and catalog-monitor lock order. Cache invalidation or catalog reset could therefore close resources still used by an operation, leak cancelled rewrite tasks through the transient-task map, or deadlock a first Hudi scan with ALTER. This change introduces an immutable Iceberg runtime context and a reference-counted writable-table lease shared by the action, transaction, and each asynchronous task. It binds reads and mutations to one exact catalog generation, releases task borrowers on every success, failure, cancellation, timeout, and pre-start path, removes cancelled tasks from the scheduler map before cancellation, and enforces lifecycle-stripe then catalog-monitor ordering for Hudi. ### Release note Fix Iceberg FileIO and Hudi filesystem-view lifecycle handling during cache eviction and catalog reset. ### Check List (For Author) - Test: Unit Test and FE build - Focused Iceberg/Hudi lifecycle, cache, action, transaction, scan, and rewrite unit tests - ./build.sh --fe - git diff --check - Behavior changed: Yes. Active Iceberg and Hudi operations retain their exact runtime generation until completion. - Does this need documentation: No
924060929
force-pushed
the
fix/cir-21497-iceberg-fileio-lifecycle-4.1
branch
from
September 1, 2026 03:34
32043d3 to
c8d6f1c
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review completed on c8d6f1c1d5cdc7201dff8b2888140af07423cb14 after two convergence rounds.
One distinct issue remains and is attached inline:
- Non-batch Hudi cancellation continues submitting already-cancelled tasks to the shared bounded file-listing executor.
Critical checkpoints:
- Catalog/HMS/Hudi lock order and generation capture: no additional distinct issue.
- Hudi fs-view ownership: the disabled-cache retirement gap remains real but is already covered by discussion
3820138581, so it is not repeated. - Iceberg catalog/FileIO/table generations, cache retirement, writable mutations, actions/transactions/rewrite tasks, schema/snapshot projections, and async paths: no additional distinct issue after duplicate fencing; the async planner lifetime is already covered by discussion
3835811078. - SplitAssignment, StatementContext, and regression-test changes: no additional distinct issue.
- User focus: none provided.
- Per review-run instructions, no builds or tests were executed.
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: Closing a statement could cancel Hudi file-system-view listing tasks while the partition submission loop kept submitting the remaining tasks. With the shared bounded file-listing executor at capacity, cancelled tasks could occupy the queue and leave the planner blocked in the rejection policy for up to ten seconds. Move task registration and submission behind ListingFsViewOwner, stop the loop after cancellation, remove cancelled queued tasks, and make the bounded rejection policy observe Future cancellation without interrupting or losing the caller interrupt state. ### Release note Fix Hudi planning cancellation so cancelled file-listing submissions stop promptly while active tasks retain their file-system-view lease until termination. ### Check List (For Author) - Test: Unit Test - HudiBatchFsViewOwnerTest, HudiScanNodeTest, ThreadPoolManagerTest - FE build and Checkstyle - Behavior changed: Yes. Hudi listing cancellation no longer submits remaining partitions or waits for cancelled tasks to enter a full queue. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete; the review converged in Round 2.
I found one blocking P1 issue: Iceberg rewrite transactions are completed directly instead of through the transaction manager that registered them, so completed or failed rewrites remain retained in the manager-local and global transaction registries. The inline comment contains the repair and test scope.
I also reviewed catalog property/replay and lock-generation atomicity, Iceberg cache/FileIO/SDK ownership, Hudi cancellation and task terminality, snapshot/schema/runtime-context coherence, writable action/transaction/rewrite async lifetimes, statement-resource closure, and the updated tests. Existing review threads were treated as duplicate fences; no other distinct issue remained after the second full pass and separate risk-focused pass. No additional user focus was specified.
Tests were not run per the review bundle instruction; this review used the authoritative diff plus targeted caller and test inspection.
### What problem does this PR solve? Issue Number: None Related PR: apache#66914 Problem Summary: Iceberg data-file rewrites registered transactions through the catalog transaction manager but committed and rolled back the underlying IcebergTransaction directly. This bypassed removal from the manager-local and global transaction registries, retaining the transaction, table, writable state, and resource generation after completion. It also left post-begin setup failures outside the rollback scope. Capture the exact transaction manager once, put all post-begin setup under its rollback scope, and complete both success and failure paths through the manager APIs. ### Release note Fix Iceberg rewrite transaction cleanup so completed and failed rewrites release their registered transaction state. ### Check List (For Author) - Test: Unit Test - RewriteDataFileExecutorTest, RewriteGroupTaskTest, IcebergTransactionTest - FE build and Checkstyle - Behavior changed: Yes. Iceberg rewrite transactions are removed from both transaction registries after commit or rollback. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for head f0189d2b421b8bea1d7c778899f6aaa21e5d4dfc
No new distinct PR-scoped finding survived full review, strict thread deduplication, and three convergence rounds, so this review adds no inline comments. Existing review discussions remain authoritative rather than being repeated here.
Critical checkpoints reviewed:
- Catalog fresh/replay ALTER, reset, close, runtime publication, and lock ordering remain lifecycle-fenced and generation-coherent on this head.
- Hudi fs-view ownership, synchronous/batch split submission, cancellation, rejection, interruption, and actual-terminal accounting retain the exact generation without publishing partial results.
- Iceberg catalog/table/cache ownership, SDK operations, FileIO, authenticators, executors, statement leases, snapshot/schema projections, scans, sinks, actions, transactions, and writable task children were traced across hit, miss, refresh, replacement, rejection, invalidation, reset, success, failure, and cancellation paths.
- Rewrite manager-local/global registries, task-manager cleanup, commit/rollback ordering, and generation lease release are balanced for the PR's changed paths.
- Generic metacache reservations, removal tokens, unpublished refresh retirement, close races, and downstream strong-value/resource hooks preserve exactly-once cleanup.
Two suspicious points were fully adjudicated. The retained-table/live-catalog write-context splice is already substantially covered by existing discussions 3835811080, 3871643612, and 3870564429, so another inline would be a duplicate. The rewrite worker false-success/delete path is technically real, but authoritative left/right comparison proves its complete trigger and consequence predate this PR; the changed lease, cancellation, and transaction-manager cleanup do not introduce or worsen it.
Review completion: all 69 changed paths and all 11,230 authoritative diff lines were covered; the final round's three normal reviewers and two separate risk reviewers all returned NO_NEW_VALUABLE_FINDINGS. No additional user focus was specified. Per the review-runner instructions, no builds or tests were run; the changed regression tests were reviewed statically.
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
3 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.
cherry pick from #66914
Problem
Iceberg table cache generations can own per-table
FileIOobjects, while branch-4.1 shares a cached HudiHoodieTableFileSystemViewacross concurrent split-planning tasks. Reclaiming either resource only through GC leaks native/off-heap state and threads, but closing it at cache eviction is also unsafe while a statement or split task is still using that exact generation.Change
FileIOcloses exactly once after the last active statement releases its lease. DLF catalog-sharedFileIOis not closed per table.Validation
./build.sh --fe -j4: all 27 reactors succeeded.git diff --check: clean.Cherry-pick note
This is the branch-4.1 variant of #66914. Master uses per-scan Hudi views; branch-4.1 keeps a shared cached view and therefore requires generation-safe reference counting and structured task completion.