Repository navigation
[improvement](fe) Defer external catalog resource cleanup from replay - #69665
suxiaogang223 merged 4 commits into
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: Slow closes of retired external catalog resources block serial journal replay. Apply metadata and cache invalidation synchronously, then reclaim captured old instances in the background. Scope connector cache retirement to its context and defer Iceberg FileIO cleanup until the last table reference is released. ### Release note Reduce journal replay delays caused by closing retired external catalog resources. ### Check List (For Author) - Test: FE build and Checkstyle passed; 113 unit tests across 10 classes passed. - Behavior changed: Yes. Retired resources close asynchronously after synchronous invalidation. - Does this need documentation: No.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69665 Problem Summary: Cover ALTER, DROP followed by same-name recreation, and rename through journal serialization and the real replay dispatcher. Verify that a blocked old connector close does not stall the next journal entry, that old metadata is retired synchronously, and that replacement metadata remains accessible before and after cleanup without relying on a cached result. ### Release note None ### Check List (For Author) - Test: Unit Test via run-fe-ut.sh --run ExternalCatalogReplayCleanupTest (8 passed); FE Checkstyle (0 violations); git diff --check. - Behavior changed: No. Test coverage only. - Does this need documentation: No.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed head def8c0b1be70f02984d3357b94b7b8d36cd951a2. Two P2 lifecycle/concurrency regressions are reported inline: a controller can be republished during DROP/rename and escape cleanup, and DROP now waits for network-bound catalog initialization while holding the global manager lock. No P0/P1 finding or existing blocking comment was identified. There were no existing inline threads and no additional user-provided focus points.
Critical checkpoint conclusions:
- Goal and evidence: logical cache retirement/property publication remain synchronous and physical resource close moves to the reclaimer. The new latch-based direct/serialized replay tests cover progress past blocked close, but miss the two interleavings reported inline.
- Scope and clarity: the change is focused on catalog/resource lifecycle, shared cache ownership and cleanup observability; the reusable owner/reclaimer boundaries are coherent.
- Concurrency and protection: query initialization/loaders/borrowers race with interactive mutations, replay/checkpoint and reclaimer workers. CatalogMgr uses its write lock, initialization/detachment use the catalog monitor, owner registration is synchronized, and cache publication/refcounts fence stale loads. Access-controller publication remains outside the catalog monitor and creates the first finding.
- Lock cost: physical connector/context/controller/FileIO close is deferred. Local cache retirement remains synchronous. Acquiring the initialization monitor under the global manager lock introduces the second finding.
- Lock order: no additional concrete lock cycle was established; the verified global-lock wait is contention on remote initialization, not a claimed deadlock.
- Lifecycle and initialization: obsolete cleanup captures exact instances; failed root construction retires its own caches; lazy siblings share their context owner; checkpoint and serving contexts with matching IDs remain isolated. Iceberg retains the catalog lease through deferred table cleanup and pins the plugin loader. The controller publication gap remains a lifecycle defect. No new cross-translation-unit static initialization applies to this Java change.
- Configuration: no settings are added; existing property updates retire and rebuild affected runtime/cache state.
- Compatibility: no persisted schema or public connector SPI change was found. Journal operation types/formats remain unchanged; dropped-object closure is transient.
- Thrift/protobuf: unchanged; cross-branch field-ID review is not applicable.
- Parallel paths: interactive and replay ALTER/DROP/rename, CREATE validation/image initialization, failed construction, root/lazy sibling, and disabled/weighted Iceberg cache paths were traced. Master and replay share the rename transition.
- Conditions: retirement, registration rejection, last-reference release, idempotent close and rejected dispatch guards correspond to concrete lifecycle states. Rejected tasks retain ownership and do not run close on the caller.
- Tests: new unit tests use latches/recording fakes for blocked cleanup, replacement isolation, late registration, failure accounting, TCCL and borrower dependencies. Serialized JournalEntity/EditLog coverage is same-process; controller republishing during removal and cold initialization during DROP are uncovered cases.
- Result files: no regression result files were changed; no executed test result is asserted by this review.
- Observability: pending/running/failure/age/duration gauges and backlog/slow/failure logs cover the new cleanup path; stages attempt later cleanup after a failure.
- Persistence and failover: existing EditLog write/replay routes are preserved, and cleanup actions carry no new durable state. Cross-FE replication/failover behavior was not executed.
- Data writes/crashes: this patch changes local resource and metadata lifecycle, not external data writes or transaction visibility. A crash discards process-local cleanup state; no new data-write atomicity protocol is introduced.
- FE/BE variables: none added or transmitted differently. BE visible versions, delete bitmaps, nullable columns and MemTracker accounting are outside the changed paths.
- Performance: queue draining avoids rescanning pending work per short close. Retirement cost and oldest-age scans scale with entries/backlog. Four workers and an unbounded pending registry retain the documented saturation limits; the global-lock amplification is actionable separately.
- Other issues and user concerns: no additional substantiated issue after the final sweep. All changed files and initial risk items have explicit dispositions.
Static inspection only: no builds, tests, benchmarks, source modifications or runtime validation were performed. External SDK close behavior and cross-FE replication remain validation limits, not unresolved review candidates. Review converged after two bounded rounds.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28424 ms |
TPC-DS: Total hot run time: 151005 ms |
ClickBench: Total hot run time: 24.31 s |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69665 Problem Summary: The catalog migration UT assumes authorization cleanup completes before migration returns. Background cleanup can still be pending, causing FE UT to fail in CI. Await the cleanup with a bounded FutureTask, propagate its assertion failures to the test caller, and check that the catalog write lock is released across all threads when cleanup runs. Journal ordering and migrated properties remain asserted. ### Release note None ### Check List (For Author) - Test: Unit Test via run-fe-ut.sh --run CatalogVarbinaryMigrationTest (7 tests); FE Checkstyle; git diff --check. - Behavior changed: No. Test synchronization only. - Does this need documentation: No.
|
run buildall |
TPC-H: Total hot run time: 28567 ms |
TPC-DS: Total hot run time: 150586 ms |
ClickBench: Total hot run time: 24.32 s |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69665 Problem Summary: DROP and rename can detach an authorization controller while the old catalog remains published, allowing a concurrent lookup to install a controller that escapes cleanup. In-flight authorization publication can also follow the reused catalog object's new name after rename. Unpublish the mappings before detachment and retain the original name throughout authorization publication and rollback. Removal also waited for network-bound initialization while holding the global catalog lock. Acquire the catalog monitor outside that lock, use a nonblocking manager-lock attempt with identity revalidation, and release the monitor before waiting on manager contention. Preserve synchronous retirement, captured-instance asynchronous close, journal ordering, post-lock cleanup, and monitored write-lock timing. ### Release note Prevent orphaned authorization controllers during catalog removal and keep unrelated catalog mutations responsive while another catalog initializes. ### Check List (For Author) - Test: Unit Test via run-fe-ut.sh across CatalogRemovalConcurrencyTest, CatalogMgrTest, CatalogVarbinaryMigrationTest, ExternalCatalogReplayCleanupTest, ExternalCatalogDeadlockTest, and AccessControllerManagerTest (49 tests); final FE Checkstyle (0 violations); git diff --check. Original-code counterchecks reproduced 10 expected failures among the first 11 new cases. - Behavior changed: Yes. DROP and rename wait for initialization outside the global catalog lock and fence old-name authorization publication. - Does this need documentation: No.
|
run buildall |
|
/review Re-check both P2 findings after the lifecycle fix. Pay particular attention to unpublishing mappings before controller detachment, original-name identity checks and rollback for custom/fallback controller publication across rename, catalog-monitor acquisition outside the manager lock, nonblocking manager-lock acquisition and snapshot revalidation, and the 13 latch/lock-contention unit cases. Also check for regressions in old/new runtime isolation and journal ordering. |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 28324 ms |
TPC-DS: Total hot run time: 151384 ms |
ClickBench: Total hot run time: 24.31 s |
|
/review |
There was a problem hiding this comment.
Reviewed all 26 changed files at e235389. No new actionable finding was verified. Both earlier P2 findings are addressed: unpublish before controller detachment and avoid waiting for initialization under the global manager lock. Neither is reposted; no existing P0/P1 finding remains to carry forward.
The two complete-coverage reviewers and separate lifecycle-risk reviewer each returned NO_NEW_VALUABLE_FINDINGS. All initial risks have evidence-backed dispositions, with no unresolved concrete candidate or uncovered static scope. No additional user review focus was supplied.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and supporting evidence | Synchronous metadata/cache retirement captures old instances; physical connector, context, controller and FileIO cleanup is dispatched. Added latch tests express replay progress and replacement isolation; their source was inspected, not executed. |
| Scope and reuse | The 14 production and 12 test files form a cohesive lifecycle change, sharing removal, ownership and reclamation helpers. |
| Concurrent actors and protection | Query initialization/loaders, DDL, serial replay and reclaimer workers interact through catalog monitors, manager locks, concurrent maps, publication gates and atomic resource references. No new concrete race was verified. |
| Work under locks | Logical retirement remains synchronous. Removal waits for initialization before acquiring the manager lock; detached physical cleanup does not wait on the replay caller. Existing synchronous initialization/invalidation limits remain. |
| Lock order | Removal uses catalog monitor then nonblocking manager tryLock; on contention it releases the monitor before waiting for the manager. Identity is revalidated. Cache remote loaders run outside publication/key gates, and retirement does not await them. No new wait cycle was established. |
| Lifecycle and initialization | Cleanup closures capture retired instances. Immediate children precede connector/context close; delayed Iceberg FileIO cleanup retains its catalog lease through completion. Plugin classloaders are pinned/restored. No static initialization dependency issue was found. |
| Cache ownership | Root and later sibling construction share the owning context; late registration is rejected and cleaned. Retirement uses exact connector owners and Env-local core containers, avoiding ID-wide connector retirement across checkpoint/serving Envs. |
| Configuration | No configuration item is added. |
| Compatibility | No persisted representation or connector SPI change requires upgrade compatibility code. |
| Thrift/protobuf | No definitions or numeric IDs are changed. |
| Parallel paths | Interactive/replay DROP and rename, ALTER, migration cleanup, custom/fallback controller publication, root/lazy siblings and direct/serialized journal replay were traced. |
| Conditional checks | Identity, closed-state, retirement and contention checks correspond to reachable lifecycle races; no unsupported defensive branch was identified. |
| Test coverage | JUnit/latch cases cover slow close/initialization, same-name recreation, rename publication, rejection, cleanup failure, late registration and borrower ordering. Same-process journal dispatch is covered in source; cross-FE replication and deployed-plugin behavior were not validated here. |
| Result files | No regression expected-output file is added or modified. |
| Observability | Pending/running/failure counts, oldest age, duration and slow/backlog/failure logging expose asynchronous reclamation. |
| Persistence and failover | Existing CatalogLog formats and EditLog routing remain; mutation/write and replay paths match. Marker migration still journals first and schedules cleanup after the outer lock. Reclamation adds no durable state. |
| Data writes and crashes | No external data-write, transaction visibility, partition-version or delete-bitmap semantics change. Cleanup ownership is process-local; a failed close attempt is logged/counted and is not implicitly retried. |
| FE/BE variables | No new transmitted variable or serialization path is introduced. |
| Performance and resources | Queue polling avoids repeated pending-set rescans during drain; saturation retains tasks without caller-runs cleanup. Backlog has no hard capacity bound, as documented by the PR. No additional concrete performance regression was verified. |
| Other issues | Further rechecks covered late loads, resource dependency ordering, failed construction, replacement cleanup and rename/controller schedules; none produced another substantiated finding. |
This is a static review only: no builds, tests, benchmarks, cluster runs or cross-FE replication checks were executed. The implementation retains the stated same-catalog initialization/cache-traversal limits and does not provide a universal query lease protocol. Review converged in one round.
Review converged after 1 round(s).
…ing it finish (#69874) ### What problem does this PR solve? Issue Number: None Related PR: #66331, #68353 Problem Summary: **In short:** dropping or altering an ADBC catalog can crash the FE process. Closing the catalog frees the native ADBC objects at once, including the connections and results that other threads are still reading, such as the column-statistics loads that a query on the catalog has just started. This kills the FE in about one External regression run in 75, whatever the PR under test changes. With this PR, closing a client only refuses new calls, and the last call still in flight releases the driver as it leaves. DROP and ALTER CATALOG still return without waiting. This commit was first pushed as the last commit of #68353, which is still in review. It shares nothing with the rest of that PR, so it is carved out here to land on its own. #### Background An ADBC catalog (#66331) reads its metadata through `AdbcClient`, which owns one native `AdbcDatabase` and one Arrow allocator per catalog. Every metadata call borrows a fresh connection through `AdbcClient.withConnection` and closes it afterwards. FE talks to the C driver through adbc-driver-jni 0.24. In that bridge, closing a parent closes everything still open under it, on the closing thread: `JniDatabase.close()` closes every connection still open on the database, and `JniConnection.close()` closes every result (`getObjects` and similar readers) still open on the connection. The bridge only checks a handle before a call starts, and the C driver does not check at all, so freeing an object that another thread is still using is a use-after-free in native code, which takes the whole JVM down rather than failing one call. Column statistics reach the driver too. When a query plans against an ADBC table, the STATS_FETCH pool loads column statistics in the background. A load that finds nothing in the statistics table asks the table itself: `PluginDrivenExternalTable.getColumnStatistic` → `AdbcConnectorMetadata.getTableHandle` → `withConnection` → `getObjects`. With `"meta.cache.adbc.metadata.enable" = "false"`, every such load calls into the driver. #### The problem, and what it cost - `AdbcClient.close()` closed the database and then the allocator immediately, whatever was still in flight. DROP CATALOG and ALTER CATALOG ... SET PROPERTIES close the catalog's connector this way, on their own thread (since #69665, on the `ExternalResourceReclaimer` pool; before that, synchronously on the statement thread, with the same race). - A statistics load that was reading a `getObjects` result, or was inside the driver on its connection, then read memory that had just been freed. Where it dies depends on where that thread is at that moment. - In CI, the External regression pipeline lost its FE in 11 of the 834 runs between 2026-10-22 and 2026-10-30. Each time, 54 to 56 suites then failed with `Communications link failure`, so the PR under test showed a red run it did not cause. All 11 died at the same moment: `test_adbc_sqlite_catalog_scan` drops its catalog with the metadata cache turned off right after querying it. In the same millisecond, fe.log shows the allocator closing with `Memory was leaked by query` and a statistics load failing with `Failed to read the ADBC getObjects result: ArrowArrayStream is already closed`. That is the same race, caught by a thread that came back only after the close had finished. | FE crash in `fe.out` | Runs | | --- | --- | | `SIGSEGV` in `nanoarrow::ArrayStreamFactory<VectorArrayStream>::get_next_wrapper+0x4` | 6 | | `SIGSEGV` in `PrivateArrowSchemaDeepCopy+0x46` | 2 | | glibc `double free or corruption (fasttop)` | 2 | | no message | 1 | | `SIGSEGV` in `sqlite3FindTable` (an earlier run, 2026-10-06) | 1 | - Users can hit this too, not only CI: DROP or ALTER an ADBC catalog while a statement or a statistics load is still reading it. #### How this PR fixes it - `AdbcClient` counts the calls inside `withConnection` (`enter` / `leave`, under the client's monitor, which also opens the database on first use). - `close()` marks the client closed, so new calls are refused with `AdbcClient has been closed`. It releases the database and the allocator itself only when no call is in flight. Otherwise it returns at once, and the last call to leave releases them. - A deferred release that fails is logged with the driver's path, as the catalog already logs a failed connector close. It is not thrown at that last caller, whose own work succeeded and which did not close anything. - `close()` never waits for the calls in flight, so DROP and ALTER CATALOG are not held up by a slow remote source. #### Results Unit tests (0 skipped). The first case runs against the real JNI bridge and the SQLite driver, the others against a recording database: | `AdbcClientTest` case | `close()` releasing at once, as before this PR | This PR | | --- | --- | --- | | a call still on its connection when the client is closed keeps using it | fails: `Native connection handle is closed` | passes | | a call still reading a `getObjects` result when the client is closed reads it to the end | fails: `the database was released under an open connection` | passes | | the last call to leave a closed client releases the database exactly once, after it leaves | fails: `the database was released under an open connection` | passes | | a release that fails as the last call leaves does not fail that call | fails: `the database was released under an open connection` | passes | | an idle client releases at once | passes | passes | Freeing only the allocator at once, and leaving the database to the last call, fails the `getObjects` case alone: `Attempting operation on allocator when allocator is closed`. A stress program (not committed) closes a client 200 times while four threads loop over `getObjects` and `getTableSchema` through `withConnection` against the SQLite driver: | | `AdbcClient` of master | This PR | | --- | --- | --- | | Runs | 5 | 5 | | JVM crashed | 5 of 5, 1.1 to 1.3 s after the JVM started: `SIGSEGV` in `PrivateArrowSchemaDeepCopy+0x40`, under `ArrowArrayStreamReader.readSchema` ← `AdbcObjectsReader.readNamespaces` ← `AdbcClient.withConnection` | 0 of 5 | | Closes completed per run | crashed before finishing | 200 | | Metadata calls per run | crashed before reporting | 6,019 to 7,238 | | Calls the close reached while they were using the driver | | 0 | | Failed closes, failed deferred releases | | 0, 0 (checked with WARN logging on, where the unit test's deliberate release failure shows up as one warning) | The stress runs used the macOS arm64 build of the same SQLite driver, so the offset differs from the Linux one in CI. #### Classes, and how they call each other - `AdbcClient`: `withConnection` brackets the body with `enter` / `leave`; `close` refuses new calls and releases only when idle; `release` closes the database, then the allocator; `openDatabase` is the one place the database is created, so a test can stand a recording database in for the driver. - `AdbcClientTest`: five new cases, one against the real driver and four against a recording database. The recording connection answers `getObjects` with a result built in memory in the client's allocator: reading a driver's result into Arrow loads arrow-c-data's JNI shim, which needs CXXABI_1.3.9 and does not load on the FE UT agent (see `AdbcNativeTestSupport`, #68510). - `AdbcObjectsReaderTest`: `response`, `catalog` and `schema` are package-private, so `AdbcClientTest` builds that result with them. ``` STATS_FETCH: ColumnStatisticsCacheLoader.doLoad -> PluginDrivenExternalTable.getColumnStatistic -> AdbcConnectorMetadata.getTableHandle (namespaces, table names) -> AdbcClient.withConnection enter() refused once closed; callsInFlight++ -> AdbcConnection.getObjects -> AdbcObjectsReader.readNamespaces / readTableNames leave() callsInFlight--; the last one out of a closed client -> release() DROP CATALOG / ALTER CATALOG ... SET PROPERTIES -> ExternalCatalog.onClose / resetToUninitialized -> ExternalResourceReclaimer -> AdbcConnector.close -> AdbcClient.close() closed = true; release() now only if no call is in flight release(): AdbcDatabase.close (closes its connections and their results) -> allocator.close ``` Not changed: `AdbcConnector`, the catalog lifecycle and `ExternalResourceReclaimer`, BE, and the regression suites; `test_adbc_sqlite_catalog_scan` stays as it is, since what it exposes is this bug.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
ALTER CATALOG properties, DROP CATALOG, and catalog rename can block the serial journal replay thread while closing old connectors, contexts, thread pools, access controllers, or Iceberg FileIO resources. A slow close delays subsequent journal entries, including entries for unrelated catalogs.
Apply metadata changes and cache invalidation synchronously, capture the retired instances, and close them through an FE resource reclaimer. The reclaimer retains ownership before dispatch, handles rejected scheduling without running cleanup on the caller, and exposes backlog, running, failure, age, and duration metrics. Iceberg FileIO cleanup runs after the last table reference is released and retains the catalog lease until cleanup finishes.
Bind connector cache retirement to the owning context, including lazy siblings and late registration, so checkpoint and serving Envs with matching catalog IDs cannot retire each other's connector caches. Clean up cache registrations and filesystem resources when root connector construction fails. Journal formats and propagation remain unchanged.
DROP and rename unpublish the old name/ID mappings before controller detachment. They wait for the catalog monitor outside the manager write lock, then try the manager lock and revalidate identity. A busy manager causes the monitor to be released before waiting and retrying, preserving compatibility with manager-first mutation paths. Controller creation, fallback publication, identity checks, and rollback use a captured catalog name so an in-flight publication cannot follow the reused object across rename. Physical cleanup still captures only retired instances.
Scope: catalog initialization under the object lock, cache traversal, quota deregistration, and row-count invalidation remain synchronous. The pending cleanup registry has no hard capacity limit, and this change does not introduce a universal query lease protocol.
The replay tests cover ALTER properties, DROP followed by same-name recreation, and rename in two forms: direct replay calls and serialized
JournalEntityentries decoded and applied throughEditLog.loadJournalon one serial caller. While the old connector close is held by a latch, the next journal entry must complete and retired cache reads must fail. Replacement metadata is read both before and after old cleanup; the FE name cache is invalidated before the second read so cached results cannot conceal a closed replacement runtime. Only catalog construction is stubbed for CREATE; registration uses the real CatalogMgr path.This is same-process replay coverage suitable for the FE UT runner. It does not validate cross-FE BDB replication. No Docker cluster, regression framework change, or documentation file is included.
Release note
Reduce journal replay delays caused by closing retired external catalog resources while preserving synchronous metadata and cache invalidation. Prevent orphaned authorization controllers during removal and keep unrelated catalog mutations responsive during slow initialization.
Check List (For Author)
Test
Validation on Fedora with JDK 17 and the published third-party prebuilt:
bash run-fe-ut.sh --run CatalogRemovalConcurrencyTest,CatalogMgrTest,CatalogVarbinaryMigrationTest,ExternalCatalogReplayCleanupTest,ExternalCatalogDeadlockTest,AccessControllerManagerTestpassed all 49 tests across 6 classes (0 failures, errors, or skips; Maven build cache disabled). The new class has 13 cases covering interactive/replay DROP and rename, controller recreation during retirement, custom/fallback publication across rename, replacement isolation, slow initialization, and the contended-lock retry path.build.sh --fe, with UI, Hive UDF and BE Java extension packaging disabled. The affected UT suite passed 113 tests across 10 classes before this follow-up.git diff --checkpassed. Cluster regression and cross-FE replication were not run locally.Behavior changed:
Does this need documentation?
Lifecycle follow-up: self-review
Check List (For Reviewer who merge this PR)