branch-4.1: [feat](cache-memory) add external metadata cache memory governance - #66717
branch-4.1: [feat](cache-memory) add external metadata cache memory governance#66717CalvinKirs wants to merge 44 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
e87de3d to
903b7bc
Compare
|
/review |
|
run buildall |
CalvinKirs
changed the title
903b7bc to
85a09c6
Compare
CalvinKirs
changed the title
|
run buildall |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes. Full-scope review completed across all 52 changed files and three convergence rounds. Two correctness blockers remain: weighted Iceberg schema/spec DDL fails on Hadoop and BaseMetastore-backed catalogs because a detached metadata base is forwarded to an identity-checking delegate, and a refreshed Paimon table can reuse a snapshot projection from an older same-ID table generation. Six additional P2 cache-efficiency/performance issues are inline: engine-wide serialization on initialized lookups, privilege-wrapper rejection, a class-wide estimator circuit, unused history-wide manifest materialization, a full metadata JSON clone on every weighted table hit, and rejection of NULL partition publication. Reservation ownership/ABA/close, Hive event copy-on-write/fencing, CREATE/ALTER/replay validation, and routing/compatibility were traced without another defect. No required AGENTS.md files, existing review threads, or additional user-provided focus were present. No builds were run because the review prompt prohibits them. Review status: converged after Round 3.
|
|
||
| @Override | ||
| public void commit(TableMetadata base, TableMetadata newMetadata) { | ||
| delegate.commit(base, newMetadata); |
There was a problem hiding this comment.
[P1] Rebind detached metadata updates before delegating the commit
When this table entry is weight-managed, publication JSON-detaches its TableMetadata, and getIcebergTable() seeds ServiceBackedTableOperations with another detached object. Iceberg 1.10.1 SchemaUpdate and BaseUpdatePartitionSpec capture ops.current() and call ops.commit(base, update) without refreshing, while HadoopTableOperations and BaseMetastoreTableOperations require base to be their current object by identity. Forwarding this clone therefore makes ALTER TABLE schema/reorder and partition evolution fail as stale for Hadoop, Hive, JDBC, Glue, and DLF catalogs whenever a table, catalog, or global weight limit enables this path. Please rebind a verified retained generation to the delegate's actual current object and cover both update kinds under weighted caching.
| return false; | ||
| } | ||
| PaimonSnapshotEntryKey that = (PaimonSnapshotEntryKey) object; | ||
| return snapshotId == that.snapshotId |
There was a problem hiding this comment.
[P1] Include the table generation in the snapshot-cache identity
The contextual value retains the fenced Paimon Table and its partition projection, but equality uses only the table name plus snapshot/schema IDs. The table entry refreshes independently, while this contextual entry cannot auto-refresh and has its own TTL. After a drop/recreate (where IDs restart) or another same-ID physical table generation, getSnapshotCache() can read the new table fence and still hit the old value, returning the old table handle and partition map. Explicit invalidation clears both entries, but ordinary table refresh/replacement does not. Please add a stable table-generation/options identity to this key or couple every table-entry replacement to snapshot invalidation, with a same-ID replacement regression test.
| } | ||
|
|
||
| @Override | ||
| public synchronized void initCatalog(long catalogId, Map<String, String> catalogProperties) { |
There was a problem hiding this comment.
[P2] Keep initialized cache lookups off the engine-wide monitor
Every ExternalMetaCacheMgr typed accessor unconditionally calls prepareCatalogByEngine, which copies and validates the properties, and then reaches this synchronized method. Even when the catalog group already exists, the lookup therefore serializes with every other catalog using this engine and repeats compatibility mapping plus hierarchy validation before computeIfAbsent discovers there is no work. This is on normal planning paths such as Iceberg table and Paimon snapshot/schema lookup, so parallel queries across unrelated catalogs acquire one global engine lock. Please add a lock-free initialized fast path and reserve synchronization/validation for the first build after create or invalidation.
| return false; | ||
| } | ||
| String className = table.getClass().getName(); | ||
| if ("org.apache.paimon.table.AppendOnlyFileStoreTable".equals(className) |
There was a problem hiding this comment.
[P2] Support the privilege wrapper before rejecting the table
A production Paimon table can be a PrivilegedFileStoreTable: Doris explicitly accepts that delegate in PaimonReaderOptions, and its schema/time-travel copies preserve the wrapper. Such a table reaches snapshot publication still wrapped, but this exact-class allowlist rejects it as unsupported_paimon_table without examining the supported underlying file-store table. With snapshot weight governance enabled the projection is then returned once but never cached, so every request reloads and re-enumerates all partitions. Please handle the approved privilege delegate chain (and account for its owned wrapper state) and cover it with a weighted-cache test.
| } | ||
| long now = System.nanoTime(); | ||
| for (Class<?> rootType : rootTypes) { | ||
| FailureCircuit circuit = FAILURE_CIRCUITS.get(rootType); |
There was a problem hiding this comment.
[P2] Do not circuit-break data-dependent failures by root class
This circuit is static and keyed only by the root Java class. After three equal incomplete results, every graph with that class is rejected for a minute before inspection. Reasons such as object_budget_exceeded and time_budget_exceeded are graph-dependent, so three large HivePartitionValues entries can make small, fully supported tables in unrelated catalogs miss the weighted cache as estimator_circuit_open; a valid sibling cannot record success because it is short-circuited here. Please scope suppression to the actual cache owner/key or only class-break deterministic unsupported-type failures, and test an invalid root followed by a valid root of the same class.
| if (sizeEstimate == null) { | ||
| sizeEstimate = MetaCacheSizeEstimator.estimateSafely("iceberg_table_preparation_failed", () -> { | ||
| icebergTable = IcebergSnapshotCacheValue.detachTableGeneration(icebergTable); | ||
| IcebergSnapshotCacheValue.materializeAllSnapshotManifests(icebergTable); |
There was a problem hiding this comment.
[P2] Avoid materializing all historical manifests for an unexposed graph
Weighted table admission now calls both manifest accessors for every historical snapshot. For v2 snapshots this lazily reads one manifest-list file per snapshot. The lists are retained only in this hidden frozen table for weighing: every public getIcebergTable() JSON-clones the metadata into a new service-backed table, dropping Iceberg's transient memoized lists, so scans cannot reuse the work. Long-lived tables therefore pay history-linear work and v2 remote I/O on each admission/refresh solely for an unused graph. Please weigh only metadata that callers can reuse, and add a many-snapshot v2 FileIO-read regression.
| } | ||
| TableOperations retainedOperations = ((HasTableOperations) retainedTable).operations(); | ||
| TableMetadata retainedMetadata = retainedOperations.current(); | ||
| TableMetadata callerMetadata = TableMetadataParser.fromJson( |
There was a problem hiding this comment.
[P2] Avoid rebuilding full metadata on every weighted cache hit
Once weighted publication freezes this table entry, every getIcebergTable() reaches these calls and serializes then reparses the complete TableMetadata: schemas, specs, sort orders, snapshots, refs, statistics, and history logs. This runs on ordinary table/scan planning and even before getSnapshotCache() can hit its contextual entry, so a successful cache hit still performs history-linear CPU and allocation and builds a transient graph outside the retained budget. Count-only entries take the early return. Please reuse a safe read-only frozen projection and construct an isolated service-backed/writable wrapper only for mutation paths, with a long-history cache-hit benchmark.
|
|
||
| IcebergPartition immutableCopy() { | ||
| return new IcebergPartition(partitionName, specId, recordCount, fileSizeInBytes, fileCount, | ||
| lastUpdateTime, lastSnapshotId, ImmutableList.copyOf(partitionValues), |
There was a problem hiding this comment.
[P2] Preserve supported NULL partition values in the immutable copy
generateIcebergPartition() intentionally appends Java null for a NULL partition field, and getPartitionRange() has a dedicated null branch, but Guava ImmutableList.copyOf rejects null elements here. With direct or inherited snapshot weight governance, publication therefore produces an incomplete estimate and serves the projection only once without caching it; every later lookup re-enumerates the partitions metadata table. Please use an ownership-isolated, null-tolerant unmodifiable copy and add a weighted snapshot-cache test with a NULL partition value.
85a09c6 to
6004a16
Compare
FE Regression Coverage ReportIncrement line coverage |
6004a16 to
0e1923f
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes. Full-scope review completed across all 70 authoritative changed files and two convergence rounds. Five new nonduplicate findings remain: three P1 correctness/lifecycle blockers (cross-FE quota replay rejection, max-weight ALTER versus first-init race, and Iceberg HadoopCatalog drop/recreate generation reuse) and two P2 cache-availability/performance issues (Iceberg Kerberos publication outside the authenticator and Paimon remote fence discovery on every cache hit). Existing eight inline discussions were deduplicated and not repeated. Reservation ownership/ABA/close, Hive event copy-on-write/fencing, strict property routing/compatibility, estimator coverage, connector wrapper chains, and Iceberg DDL/DML/action invalidation were traced without another defect. No required AGENTS.md files or additional user-provided review focus were present. No builds were run because the review prompt prohibits them. Review status: converged after Round 2.
| if (parsed <= 0) { | ||
| throw new IllegalArgumentException(CATALOG_MAX_WEIGHT_PROPERTY + " must be positive"); | ||
| } | ||
| if (globalMaxWeight.isPresent() && parsed > globalMaxWeight.getAsLong()) { |
There was a problem hiding this comment.
[P1] Do not reject replayed catalogs against this FE's local global cap. external_meta_cache_max_weight is per-FE and may be a percentage of local heap, while meta.cache.max-weight is persisted after validation only on the master. For example, a 4 GB catalog cap accepted with global=20% on a 32 GB master will fail every lazy cache initialization on an 8 GB observer, because replay skips DDL validation and this check runs on access. Please let the local global bucket clamp the effective admission limit (while keeping DDL hierarchy validation), and cover heterogeneous-heap replay.
| if (sizeEstimate == null) { | ||
| sizeEstimate = MetaCacheSizeEstimator.estimateSafely("iceberg_table_preparation_failed", () -> { | ||
| icebergTable = IcebergSnapshotCacheValue.detachTableGeneration(icebergTable); | ||
| IcebergSnapshotCacheValue.materializeCurrentSnapshotManifests(icebergTable); |
There was a problem hiding this comment.
[P2] Keep manifest materialization inside the catalog authentication scope. The loader's getExecutionAuthenticator().execute(...) ends after ops.loadTable(), but weighted preparation later calls dataManifests(table.io()) / deleteManifests(table.io()) here. For the Kerberized Hadoop catalog, credentials are supplied only inside HadoopExecutionAuthenticator.execute, so this manifest-list read can fail; estimateSafely then marks the value incomplete and every weighted table lookup is returned uncached (the snapshot estimator has the same problem). Please run remote-I/O preparation under the owning catalog authenticator, with a credential-scoped admission/hit regression.
| } | ||
| Snapshot snapshot = metadata.currentSnapshot(); | ||
| long snapshotId = snapshot == null ? IcebergUtils.UNKNOWN_SNAPSHOT_ID : snapshot.snapshotId(); | ||
| return Optional.of(new IcebergSnapshotEntryKey(nameMapping, metadata.metadataFileLocation(), |
There was a problem hiding this comment.
[P1] Include the physical table generation in this key. HadoopCatalog reuses the deterministic metadata/v1.metadata.json path after a purged same-name drop/recreate, and an empty replacement also resets snapshot/schema/spec IDs to -1/0/0; its UUID is new, but every field here collides. After the table entry refreshes, this contextual entry can therefore return the old retained table. The same collision also passes isSameGeneration(), which accepts equal locations without checking UUID. Please key/fence on UUID or a table-entry generation and cover an empty HadoopCatalog drop/recreate.
| ExternalMetaCacheMgr extMetaCacheMgr = Env.getCurrentEnv().getExtMetaCacheMgr(); | ||
| ExternalMetaCacheMgr extMetaCacheMgr = Env.getCurrentEnv().getExtMetaCacheMgr(); | ||
| if (java.util.Objects.nonNull(schemaCacheTtl) | ||
| || updatedProps.containsKey(ExternalMetaCacheBudgetManager.CATALOG_MAX_WEIGHT_PROPERTY)) { |
There was a problem hiding this comment.
[P1] Fence this new quota invalidation against an in-flight first initialization. prepareCatalogByEngine() can copy the old properties while no group exists; if ALTER commits this setting next, removeCatalog() skips the absent group, and the delayed initializer then publishes the old count-only policy indefinitely. That silently defeats the configured memory bound. Please version/serialize the property snapshot with removal and publication, and add a paused ALTER-vs-init test that verifies the new weighted policy wins.
| return tableEntry.get(nameMapping.getCtlId()).get(nameMapping).getLatestSnapshotCacheValue(); | ||
| PaimonTableCacheValue tableValue = tableEntry.get(nameMapping.getCtlId()).get(nameMapping); | ||
| Table table = tableValue.getPaimonTable(); | ||
| PaimonSnapshot fence = latestSnapshotProjectionLoader.loadFence(nameMapping, table).getSnapshot(); |
There was a problem hiding this comment.
[P2] Avoid resolving the remote fence before every snapshot-cache lookup. loadFence() runs before snapshotEntry.get(), and its path calls copyWithLatestSchema(), latestSnapshot(), and schemaManager().latest(), so even a hit on an admitted snapshot still performs latest-metadata discovery. Before this change, PaimonTableCacheValue memoized the projection, so stable repeated reads avoided that work. Please retain or refresh the fence under the table generation (or otherwise put discovery behind a cache) and add a repeated-hit call-count test.
FE Regression Coverage ReportIncrement line coverage |
0e1923f to
57a8d5d
Compare
|
run buildall |
|
/review |
|
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. |
FE UT Coverage ReportIncrement line coverage |
|
Design suggestion after reviewing the current head
For these goals we do not need a precise retained-object-graph size. I suggest using long weight = BASE_WEIGHT
+ partitionCount * PARTITION_WEIGHT
+ fileCount * FILE_WEIGHT;The unit can be approximate KiB. The constants can be calibrated offline with the existing benchmarks/full estimator and rounded up to simple powers of two. Production weighing should only read O(1) collection sizes or counters already produced by the normal loader. It must not reflect over object fields, build an identity set, materialize lazy state, read manifests remotely, or serialize/clone the value. Suggested formulas for the currently managed and adjacent unbounded entries: For Paimon, The formula should include only collections actually retained by that cache value. Hudi, MaxCompute, Doris, Hive single-partition, and other small/bounded entries can remain count-based until one of their values retains an unbounded collection; then the same
There is one important lifecycle requirement in the current implementation: I would keep the existing global/catalog/entry budget hierarchy, admission-before-publication protocol, generation fencing, and rejection behavior. I would replace the production The key tests should be:
This gives a substantially cheaper and more maintainable implementation while still improving by orders of magnitude over pure entry-count limits. As with the current PR scope, it controls retained MetaCache memory after load; it does not bound temporary memory used by the query that constructs the value before admission. |
FE Regression Coverage ReportIncrement line coverage |
57a8d5d to
b89e044
Compare
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…ing the fence per query The fence-keyed latest model read the snapshot fence from storage on every latest lookup, so a plain query observed external commits immediately whenever the SDK layer did not serve a stale snapshot. That intermittently broke the established external metadata cache contract (latest metadata stays as stale as the cached table handle until TTL/refresh, verified by the merged test_paimon_table_meta_cache regression) and paid one snapshot read per query. getSnapshotCache now serves the projection of the most recently observed fence while it is still published for the current table generation, and re-observes the fence only when no projection of the generation is reachable anymore (first read, expiry, weight eviction, explicit invalidation). Staleness and IO now match the memoized pre-governance behavior, while fence observation ordering keeps handling concurrent re-observation and rollback replacement exactly as before.
…ispatch-generation overload IcebergMetadataOps DDL sites now acquire writable tables through the two-argument dispatch-generation overload; the validation, branch/tag and DDL-plan suites still stubbed only the single-argument form, so the real method ran without an Env and failed (and the leaked static mock state could poison unrelated tests sharing the fork).
|
run buildall |
|
/review |
FE Regression Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Two new issues remain after duplicate-fencing against all existing review threads:
- a statement-pinned Iceberg table can be planned with a later catalog generation's authenticator, storage state, and pre-auth executor;
- the newly retained Iceberg/Paimon authentication generations are omitted from otherwise-complete byte estimates.
The shared reservation/removal lifecycle, cache-policy handoff, FE/BE stats compatibility, replay/validation behavior, other connector paths, tests, and benchmark wiring produced no additional non-duplicate findings. The residual Paimon validation-to-admission race was not re-submitted because the existing acquisition thread already requires its lifecycle fence to extend through publication.
User focus: no additional user-provided review focus was supplied.
No builds or tests were run, as required by the bundle-only review instructions.
| ((IcebergMvccSnapshot) snapshot.get()).getSnapshotCacheValue(); | ||
| if (cacheValue.getIcebergTable().isPresent()) { | ||
| Table frozenBaseTable = cacheValue.getIcebergTable().get(); | ||
| Optional<Table> frozenTable = cacheValue.getIcebergTable(); |
There was a problem hiding this comment.
[P1] Keep the frozen table on its generation's execution context
This selects the relation-pinned G1 table, but doInitialize() independently captures the catalog's current authenticator/storage properties and createTableScan() uses its current pre-auth pool. A statement bound before a credential/storage ALTER can therefore plan G1 FrozenTableOperations/FileIO under A2 and the G2 executor after reinitialization. The table-value authenticator used while building the projection is not carried into IcebergSnapshotCacheValue/IcebergMvccSnapshot. Please retain the generation-owned authenticator and executor (or fail before planning if that generation retired), and add a bind -> ALTER/reinitialize -> scan latch test, including a snapshot-selectable system table.
There was a problem hiding this comment.
Fixed in 246b544: Iceberg snapshot projections now carry the ExecutionAuthenticator captured from the table generation that built them (bound at both projection branches in getSnapshotCache), and IcebergScanNode.useFrozenTableGeneration - the shared entry for regular relations and snapshot-selectable system tables, called from both doInitialize and createTableScan - validates the pinned projection against the catalog current context via ensurePlannableUnder before any frozen table is planned. A statement bound before a credential/storage ALTER now fails retryably before planning instead of running G1 FrozenTableOperations/FileIO under the replaced authenticator, storage state and executor. Regressions cover context propagation from the published generation and the fence accepting the captured context while rejecting a replaced one (system tables share the same validated path).
| IcebergTableCacheValue value = execute(authenticator, () -> { | ||
| Table table = ops.loadTable(nameMapping.getRemoteDbName(), nameMapping.getRemoteTblName()); | ||
| IcebergTableCacheValue loaded = new IcebergTableCacheValue(table); | ||
| loaded.bindAuthenticator(authenticator); |
There was a problem hiding this comment.
[P2] Account the retained authentication generation
This binding makes an admitted table value a strong owner of the catalog generation's authenticator. For Kerberized catalogs that graph includes the Hadoop authenticator/configuration, Subject/UGI, and credential collections. A credential/storage ALTER clears the catalog reference without retiring this cache group, so old values can retain old authentication generations, but the entry estimator still returns complete without charging them; the new Paimon table/snapshot bindings have the same ownership gap. Since the authenticator is shared, please charge it once at a catalog/generation owner while any admitted value retains it (or fail weighted admission closed when it cannot be bounded), and cover credential growth across reset/rotation.
There was a problem hiding this comment.
Fixed in 246b544: both the Iceberg and Paimon estimators now add a flat rounded-up retained-context allowance (AUTHENTICATION_CONTEXT_WEIGHT = 16KB, mirroring the existing per-owner FileIO and encryption-manager allowances of the coarse model) to every admitted table or snapshot value that carries a bound execution context, so a retired authentication generation kept alive only by old cached values is never entirely unaccounted; per-value charging matches the established independent-owner-lifetimes accounting stance. Regression asserts the exact allowance delta for bound vs unbound table and snapshot values in both engines.
…d charge the retained context A statement pinned to a frozen Iceberg generation planned that generation's FrozenTableOperations/FileIO with whatever authenticator, storage state and pre-authenticated executor the catalog currently served: doInitialize captures the current context independently of the relation-pinned table, so a credential/storage ALTER between binding and planning spliced the generations. Iceberg snapshot projections now carry the ExecutionAuthenticator captured from the table generation that built them, and IcebergScanNode.useFrozenTableGeneration - the shared entry for regular relations and snapshot-selectable system tables - validates it against the catalog's current context before planning; a replaced context fails the statement retryably instead of planning a spliced scan. Estimator side, an admitted table or snapshot value is a strong owner of its generation's execution context (for Kerberized catalogs that graph includes the Hadoop authentication state and credential collections), and a retired generation can stay alive only through such values. Both the Iceberg and Paimon estimators now add a flat rounded-up retained-context allowance (16KB, mirroring the existing FileIO and encryption-manager allowances) to every value that carries a bound context, so those retentions are never entirely unaccounted. Regressions: allowance deltas for bound vs unbound table and snapshot values in both engines, projection context propagation from the published generation, and the planning fence accepting the captured context while rejecting a replaced one.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one new P1 generation-binding gap remains.
The explicit Iceberg VERSION/TIME and branch/tag snapshot path retains the table generation without its captured execution context, so a credential/storage ALTER can bypass the new retry fence. See the inline comment.
Review checkpoints:
- Reviewed the authoritative 98-file bundle at head
246b5445097c18e66c527718aa01099c80739eb6; the live base/head pair was reverified immediately before submission. - Audited cache admission/accounting, catalog property and lifecycle fencing, Hive generation/event handling, Iceberg/Paimon generation, authentication and cleanup, BE/FE compatibility, estimators, benchmarks, unit tests and regressions.
- Completed two review rounds; all three Round 2 reviewers returned no new valuable findings, and every earlier candidate was accepted, dismissed with evidence, or fenced by an existing thread.
- Deduplicated against the thread digest, all raw inline comments, and a final live comment query; no substantially similar new inline was repeated.
- No additional user review focus was provided.
- Builds and tests were not run, as required by the review-runner contract.
| // If a snapshot is specified, use the specified snapshot and the corresponding schema (not latest). | ||
| Table icebergTable = IcebergSnapshotCacheValue.retainTableGeneration(getIcebergTable(dorisTable)); | ||
| IcebergExternalMetaCache metaCache = icebergExternalMetaCache(dorisTable); | ||
| Table icebergTable = metaCache.getQueryScopedIcebergTable(dorisTable); |
There was a problem hiding this comment.
[P1] Carry the generation context through explicit snapshots
This branch now retains the query-scoped G1 table, but it returns only Table; the IcebergSnapshotCacheValue constructed below never calls bindCapturedAuthenticator. Both IcebergExternalTable.loadSnapshot() and the HMS-Iceberg sibling wrap that value directly, so after a credential/storage ALTER ensurePlannableUnder(A2, ...) sees captured == null and permits the G1 frozen operations/FileIO to be planned with current A2/storage/P2. The two bindings added in IcebergExternalMetaCache.getSnapshotCache() do not cover VERSION/TIME or branch/tag relations. Please carry the table and its captured execution context together here, and add bind -> ALTER/reinitialize -> plan regressions for an explicit snapshot and a ref.
There was a problem hiding this comment.
Fixed in the follow-up commit: the explicit VERSION/TIME and branch/tag path now resolves the generation once (getTableCacheValue exposing the handle and its captured context together), derives the query-scoped table from that same value, and newExplicitSnapshotValue binds the generation context onto the constructed IcebergSnapshotCacheValue - so IcebergExternalTable.loadSnapshot() and the HMS-Iceberg sibling hand the planning fence a bound value, and ensurePlannableUnder rejects a catalog reset between binding and planning for explicit snapshots and refs exactly like latest projections. Regression testExplicitSnapshotValueCarriesItsGenerationContext covers the binding and the fence rejecting a replaced context for a ref.
…TIME and branch/tag snapshots The explicit-snapshot path resolved a query-scoped table of the current generation but built its IcebergSnapshotCacheValue without the captured execution context, so the planning fence saw no binding and permitted a relation bound before a credential/storage ALTER to plan the old generation's frozen operations and FileIO under the replaced context. The generation is now resolved once - handle and captured authenticator together - and newExplicitSnapshotValue binds that context onto the constructed value, putting VERSION/TIME and branch/tag relations on the same planning fence as latest projections. Regression covers binding and the fence rejecting a replaced context for an explicit ref.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for two remaining issues that make catalog transition behavior fail to converge:
- Iceberg snapshot projections can remain bound to a retired authenticator after an auth-only catalog reset and same-metadata refresh, so the planning fence rejects every retry until cache expiry or invalidation.
- A lookup after permanent catalog drop still waits the entire two-second handoff retry window, even after lifecycle preparation has confirmed there is no catalog.
Review checkpoints:
- Correctness/lifecycle: cache-budget ownership, removal/refresh/close/reclaim interleavings, catalog create/ALTER/rollback/replay/rename/drop, and retained engine resources were audited; the two issues above are the new actionable results.
- Compatibility/configuration: FE/BE statistics schema ordering, rolling fallback, configuration routing/sanitization, and estimators were checked. Remaining concerns in those areas are either covered by existing review threads or disproved.
- Connector coverage: Hive, Iceberg, Paimon, Hudi, MaxCompute, and Doris cache routes plus Iceberg scan/action/transaction/system-table paths were reviewed.
- Tests: please add the two regressions requested inline; no builds were run because this runner is review-only.
- Existing context: live threads and raw PR comments were used as hard duplicate fences.
- User focus: no additional focus was supplied.
|
|
||
| private void retireTableGeneration(NameMapping nameMapping, | ||
| @Nullable IcebergTableCacheValue previousValue, IcebergTableCacheValue currentValue) { | ||
| if (previousValue != null && previousValue.isSameOperationalGeneration(currentValue)) { |
There was a problem hiding this comment.
[P1] Retire projections when the authenticator changes
An auth-only catalog ALTER resets A1 to A2 without retiring the Iceberg cache group. After the table entry refreshes the same UUID/metadata file and equivalent FileIO resources under A2, this equality check still returns early because the captured authenticator is not part of the operational generation; the hit-side sharesOperationalResources check has the same omission. The snapshot therefore remains bound to A1, IcebergScanNode correctly rejects it under A2, and every retried statement keeps hitting the same rejected projection until expiry or explicit invalidation. This is the recovery path beyond the existing scan-fence thread: failing safely is not retryable if the stale projection survives. Please include authenticator identity in replacement and hit revalidation (or retire the group on operational ALTER), with an A1 -> auth-only A2 -> same-metadata refresh regression.
There was a problem hiding this comment.
Fixed in the follow-up commit: the captured execution context is now part of the Iceberg operational generation - isSameOperationalGeneration compares authenticator identity, and hit-side sharesOperationalResources revalidation compares the table generation context against the projection captured context - so an auth-only ALTER followed by a same-UUID/metadata refresh with equivalent FileIO retires the old-context projection on replacement, and a hit rebuilds it bound to the new context instead of serving a permanently unplannable value. The planning fence also now applies only when a frozen handle is actually planned, so count-mode values are unaffected. Regression testAuthOnlyAlterRetiresProjectionsOfTheOldContext covers A1 -> auth-only A2 -> same-metadata refresh at both the equality and replacement-retirement levels.
| // before without any deadlock. | ||
| long deadlineNanos = System.nanoTime() + PREPARE_RETRY_WINDOW_NANOS; | ||
| while (true) { | ||
| catalogPreparer.accept(catalogId); |
There was a problem hiding this comment.
[P2] Stop retrying once the catalog is permanently gone
After DROP CATALOG, the preparer can acquire the lifecycle stripe and confirm that no catalog remains, but this void callback cannot distinguish that terminal state from lock contention. A retained-table/schema lookup therefore sleeps in this loop for the full two-second window (about forty retries) before getSchemaCacheValue() can return its intended Optional.empty(). This is distinct from the existing contended-ALTER thread. Note that ALTER CATALOG ... RENAME also creates a transient map absence, so treating every null lookup as terminal would regress that handoff; please propagate a permanent-drop/tombstone outcome (while preserving retry for rename/contended cases) and cover both cases with latency-bounded tests.
There was a problem hiding this comment.
Fixed in the follow-up commit: onCatalogPermanentlyRemoved now records a tombstone in AbstractExternalMetaCache (catalog ids are never reused; defensively cleared if the id is ever re-initialized), and requireCatalogEntryGroup consults it before and during the bounded retry, so a lookup after DROP CATALOG fails immediately with a dropped-catalog message instead of sleeping through the two-second window, while rename and contended-ALTER handoffs (no tombstone) keep the existing bounded retry. Both engine overrides of the hook call super so the tombstone is engine-wide. Latency-bounded regression testPermanentlyDroppedCatalogFailsLookupsImmediately covers the terminal drop (well under the window, preparer not spun) and the transient-absence retry that still re-prepares.
…s and fail dropped-catalog lookups terminally An auth-only catalog ALTER hands out the same metadata and operationally equivalent FileIO under a new execution context. The captured authenticator was not part of the Iceberg operational-generation equality, so replacement kept the old projection, hit revalidation kept serving it, the planning fence kept rejecting it, and every retried statement failed until expiry. The captured context is now part of isSameOperationalGeneration and of hit-side revalidation, so the refresh retires old-context projections and the rebuilt one is plannable again; the planning fence itself now applies only when a frozen handle is actually planned (count-mode values plan the live table). After DROP CATALOG the group preparer could never restore the group, but the bounded contended-handoff retry could not distinguish that terminal state from lock contention and slept through the full two-second window before failing. Permanent removal now records a tombstone (catalog ids are never reused; defensively cleared on re-init) that fails lookups immediately with a dropped-catalog message, while rename and contended handoffs keep the bounded retry. Regressions: same-metadata refresh under a new context retires the old projection (with operational-equality assertions), and dropped-catalog lookups fail in far under the retry window while a transiently absent group is still re-prepared.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for five distinct issues found after two convergence rounds and a final full-diff sweep:
- [P1] Operational credential/storage ALTERs leave cached Iceberg/Paimon base generations bound to the closed execution context.
- [P2] Paimon can let an older captured latest fence overwrite a newer physical observation.
- [P2] Dependency retirement can globally delay quota release for already-removed values.
- [P2] DROP publishes its terminal tombstone only after synchronous group cleanup.
- [P2] Each engine permanently retains every dropped catalog ID outside cache governance.
I reviewed all 98 changed paths and deduplicated these findings against the 110 live inline comments. There was no additional user-provided focus. Per the review instruction, I did not run builds or tests.
| routeCatalogEngines(catalogId, cache -> safeInvalidate( | ||
| cache, catalogId, "removeCatalogPermanently", | ||
| () -> cache.invalidateCatalog(catalogId))); | ||
| for (ExternalMetaCache cache : cacheRegistry.allCaches()) { |
There was a problem hiding this comment.
[P2] Publish the permanent-drop state before closing routed groups. CatalogMgr has already removed the catalog, and invalidateCatalog() detaches an engine group before synchronously closing it. While that close (or another engine's close) is blocked, a retained lookup sees no group and no catalog but still has no tombstone, so it sleeps/retries for the full two-second handoff window. This is the ordering gap left in the terminal-retry fix. Please mark DROP terminal before detachment/close, while keeping any post-close engine cleanup separately fenced, and latch-test a lookup during a blocked close.
There was a problem hiding this comment.
Fixed in bfca15a together with the membership concern: the terminal state is no longer published by the engine hook at all - the lookup probes the catalog manager, which removes the catalog before removeCatalogPermanently detaches or closes any engine group, so a retained lookup during a blocked close already observes the drop and fails immediately. Post-close engine cleanup stays where it was, and rename keeps the catalog registered so its transient absence still gets the bounded retry.
| NameMapping nameMapping = dorisTable.getOrBuildNameMapping(); | ||
| return tableEntry.get(nameMapping.getCtlId()).get(nameMapping).getLatestSnapshotCacheValue(); | ||
| IcebergTableCacheValue tableValue = | ||
| tableEntry.get(nameMapping.getCtlId()).get(nameMapping); |
There was a problem hiding this comment.
[P1] Retire the cached base generation when an operational catalog property changes. Every property update resets the catalog authenticator and closes its SDK resources, but credential/storage keys do not remove the Iceberg or Paimon cache group. A brand-new Iceberg statement can therefore get the admitted A1 table here, accept its matching A1 projection, and then fail the A2 planning fence; retries keep hitting A1 until managed refresh is due. Paimon's memoized latest path can keep returning the old closed generation without a live-context check. Please retire the affected engine group (or synchronously validate/reload its base) on execution-context changes, and test an immediate post-ALTER statement without manually publishing A2 first.
There was a problem hiding this comment.
Fixed in bfca15a: CatalogMgr now calls ExternalMetaCacheMgr.onCatalogOperationalContextChanged after the committed ALTER (fresh and replayed), which retires the routed engines cached entries (groups and policies stay) - Iceberg base tables, snapshot/schema projections and Paimon memoized latest state included - so an immediate post-ALTER statement loads a generation bound to the new context instead of retrying against the unplannable A1 base. Failed validations retire nothing. Regression testCommittedAlterRetiresTheOperationalContextButFailedAlterDoesNot covers both paths.
|
|
||
| @Override | ||
| public void onCatalogPermanentlyRemoved(long catalogId) { | ||
| permanentlyRemovedCatalogs.add(catalogId); |
There was a problem hiding this comment.
[P2] Avoid retaining every dropped catalog ID forever in every engine. removeCatalogPermanently() calls this hook for all registered caches, even engines that never had a group, while the only removal is initCatalog() and the DROP contract says IDs are never reused. Catalog churn therefore grows a boxed ID/map node per engine for the FE lifetime outside the new budgets. Please return a terminal outcome from the manager/preparer (or use bounded in-progress DROP state) instead of permanent per-engine membership, and add a churn/cardinality regression.
There was a problem hiding this comment.
Fixed in bfca15a: the per-engine dropped-id membership is gone. requireCatalogEntryGroup now derives the terminal outcome from the catalog manager (ids are never reused and the manager removes the catalog before any engine detachment), so catalog churn retains no per-engine state at all; a test probe hook replaces the manager lookup in isolated tests. The drop/rename regression asserts immediate terminal failure and the preserved transient-absence retry.
|
|
||
| private void drainRemovalCleanups() { | ||
| try { | ||
| drainRemovalNotifications(); |
There was a problem hiding this comment.
[P2] Release dead reservations before running dependency retirement. Every entry shares this single cleanup executor, and drainRemovalNotifications() runs first. A Paimon table notification scans and invalidates all snapshot/schema children, so one large callback can hold this worker while automatic removals in this and unrelated catalogs remain charged in pendingRemovalGenerations. During that interval new admission can be rejected or live peers reclaimed even though the parent values are already gone. Please let reservation cleanup progress independently (or at least before potentially unbounded listeners), and cover a blocked listener while an unrelated automatic eviction releases its global/catalog quota.
There was a problem hiding this comment.
Fixed in bfca15a: drainRemovalCleanups now releases dead reservations before running dependency-retirement listeners, and the removal callback queues the reservation before the notification so the worker can never observe the notification without the matching quota release. Regression testDeadReservationsReleaseBeforeDependencyRetirement holds a listener on a latch after an automatic eviction and asserts the global quota is already released while the listener is still blocked.
| // Order fence observations, not snapshot ids: a rollback moves the latest snapshot | ||
| // backwards, and a concurrent call may finish after a later observation (reversed | ||
| // completion). Either way the most recently observed fence is the one future lookups read. | ||
| long observation = fenceObservations.incrementAndGet(); |
There was a problem hiding this comment.
[P2] Assign this ordering at the fence-capture boundary. A can capture fence 8 at line 136 and pause before this increment; B can then capture fence 9, get observation N, and publish it; when A resumes it gets N+1, replaces B in latestObservedFences, and retires the fence-9 projection. Subsequent memoized latest reads then return the older fence 8. The reversed-completion test pauses during projection enumeration, after this number is assigned, so it misses this window. Please serialize capture plus sequence assignment per owner and add the capture-before-counter interleaving.
There was a problem hiding this comment.
Fixed in bfca15a: fence capture and observation-number assignment are serialized per owner (fenceCaptureLocks, entries retired together with their owners across generation retirement, unpublished-generation cleanup and catalog purges), so the observation order always matches the fence-read order and a capture pausing between read and increment can no longer replace a newer published fence with an older one. Regression testFenceCaptureAndObservationAssignmentAreSerializedPerOwner blocks a capture inside the fence read (before the counter), races a newer capture, and asserts serialization plus the newest-read fence owning the memoized latest.
…d fence capture Committed catalog property ALTERs reset the execution context and close SDK resources without retiring engine cache groups, leaving cached base generations that every planning fence rejects until managed refresh. CatalogMgr now notifies ExternalMetaCacheMgr.onCatalogOperationalContext- Changed after the commit (fresh and replayed), which retires the routed engines' cached entries - groups and policies stay - so the next statement loads a generation bound to the new context. Failed validations retire nothing. The dropped-catalog terminal state no longer uses per-engine dropped-id membership (which grew per engine for the FE lifetime and was published only after groups were detached and closed): the lookup now probes the catalog manager, which removes the catalog before any engine detachment, so a lookup during a blocked close fails immediately while rename and contended handoffs - where the catalog stays registered - keep the bounded retry. No per-engine state is retained. The shared removal-cleanup worker now releases dead reservations before running dependency-retirement listeners, and the removal callback queues the reservation before the notification, so one expensive listener can no longer keep already-freed quota charged while admissions are rejected or peers reclaimed. Paimon fence capture and observation-number assignment are serialized per owner: a capture pausing between reading the fence and taking its number could otherwise outnumber a later capture that already published a newer fence and replace it with the older one; lock entries retire with their owners. Regressions: post-ALTER retirement wiring (and none on failed validation), immediate dropped-catalog failure with preserved transient-absence retry, quota release while a listener is blocked, and serialized capture with the newest-read fence owning the memoized latest.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found one blocking failure-path lifecycle issue in the new Paimon latest-fence serialization. The cache accounting, catalog lifecycle, Hive/Iceberg/Paimon generation handling, schema compatibility, estimators, and related tests were otherwise reviewed with no additional non-duplicate findings.
| // and the increment could replace a newer already-published fence with an older one. | ||
| PaimonSnapshot fence; | ||
| long observation; | ||
| Object captureLock = fenceCaptureLocks.computeIfAbsent(owner, ignored -> new Object()); |
There was a problem hiding this comment.
[P2] Clean up capture locks when projection loading fails
After this computeIfAbsent, both loadLatestSnapshotFence() and the later projection loader can throw before the unpublished-generation cleanup at lines 180-187. tables.get() still returns a freshly generated table value when weighted admission rejects it, so repeated failures for an oversized/unsupported table strand a different fenceCaptureLocks owner on every request; those rejected values have no replacement/removal callback, leaving this map outside all configured cache budgets. Please cover capture through projection publication with failure cleanup that conditionally removes the unpublished owner (preferably remove(owner, captureLock)), and add repeated rejected-load tests for both exception points.
There was a problem hiding this comment.
Fixed in the follow-up commit: capture through projection publication now runs under a finally block that performs the unpublished-generation cleanup on every path - including the fence-read and projection-load failure points - conditionally removing the exact registered lock via remove(owner, captureLock) together with the observed-fence owner. Regression testFailedFenceOrProjectionLoadsDoNotStrandCaptureLockOwners drives repeated weight-rejected loads through both exception points and asserts both maps end empty.
…lure paths The fence read or the projection load can throw before the unpublished-generation cleanup ran, and a weight-rejected table produces a fresh generation - and therefore a fresh capture-lock owner - on every lookup, so repeated failures for an oversized or unsupported table stranded fenceCaptureLocks entries outside every configured budget. Capture through projection publication now runs under a finally that performs the unpublished-generation cleanup on all paths, conditionally removing the exact registered lock. Regression drives repeated rejected loads through both failure points and asserts neither the lock map nor the observed-fence map retains an owner.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed against b090024bacd197aa013e24bf69921b19f1896ffc after three bounded convergence rounds.
No new non-duplicate inline findings remain. The final Paimon lifecycle candidate—owner resurrection after catalog-invalidation cleanup—was reclassified as substantially covered by existing thread r3800214550 after refreshing the full raw comment body. Other residual concerns likewise map to existing review threads; this review does not clear or supersede those discussions.
Critical checkpoints:
- Memory governance and concurrency: reservation, reclaim, close, replacement, and delayed-removal paths were reviewed; no additional distinct issue survived the existing-thread duplicate fence.
- Catalog and connector lifecycle: property publication/rollback/operational-context handling and Iceberg/Paimon generation, authentication, and resource handoffs were checked. The rename transient-absence concern remains covered by existing thread r3836799898.
- FE/BE compatibility: the 33-column schema mapping aligns positionally and legacy rows are NULL-filled consistently; the existing any-error fallback concern was not duplicated.
- Estimator and test coverage: estimator formulas, SDK-cache controls, unit tests, regression tests, and benchmarks were reviewed statically. No builds were run, per the review contract.
- User focus: no additional focus was provided.
Outcome: comment-only review with intentionally zero new inline comments. Existing change-request threads remain authoritative.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
3 participants
DRAFT Docs
https://github.com/CalvinKirs/doris-website/blob/2125f053594b821a6ab7556f035b9cb1e5b43a0f/i18n/zh-CN/docusaurus-plugin-content-docs/version-4.x/lakehouse/external-meta-cache-memory-management.md
apache/doris-website#4061 (comment)
Summary
Add retained-memory governance for selected external metadata caches. Existing count-based capacity remains the default. Weighted admission is enabled only for an estimator-backed entry when at least one applicable global, catalog, or entry memory limit is configured.
Why
Managed scope
partition_values.table,snapshot, andmanifest(manifestremains disabled by default).snapshot.Other external metadata entries continue to use their existing count-based behavior.
Accounting and ownership strategy
Iceberg table/snapshot cache values use a detached, non-growing metadata generation. Historical refs/snapshots/statistics are not retained by the cache entry; a statement that needs them reads the exact pinned metadata file into a query-local table under the catalog authenticator. The statement keeps one generation even if the cache concurrently refreshes. A stale unbound cache generation is invalidated and retried once; an already-bound statement fails instead of silently switching generations. Snapshot identity includes
metadataFileLocation + snapshotId + schemaId + defaultSpecId.Paimon partition payload bytes are accumulated in the existing partition-construction loop, including every retained typed value and display name. This avoids sampling misses without a second full traversal.
Limit behavior
Configuration
external_meta_cache_max_weight=10GBor20%;0disables the FE-global quota.meta.cache.max-weight=4GB.meta.cache.<engine>.<entry>.max-weight=1GB.Not every entry needs an explicit limit. Estimator-backed entries inherit the nearest configured parent. Catalog/entry limits also work when the FE-global limit is disabled. The hierarchy is validated as
entry <= catalog <= globalwhen the corresponding parents exist. Unknown engines, entries, options, aliases, and max-weight on entries without an estimator are rejected during catalog validation.Optimizer and query-path impact
No optimizer rule, literal representation, partition-item implementation, or system-table exposure is added. The only scan-node edit stores an existing
Optionalresult once before use; it does not change scan planning semantics.Validation
git diff --checkpasses.520964 <= 524288; budget rejection did not fail queries; no incomplete estimate, accounting underflow, deadlock, or OOM was observed. The latest source behavior is covered by the focused unit regression above.Performance results
In-repo benchmark harness, Java 17,
-Xms1g -Xmx4g, 500 ms warmup and 3 x 500 ms measurement. Results are per operation.The Iceberg comparison includes
DataFile.copy()in both paths, matching the production manifest reader. Even in the dense-metrics stress cases, copying/parsing remains the larger component than the incremental counter. Iceberg table publication is 4.401 us (10 fields) / 9.991 us (100 fields); 1k versus 10k retained snapshot history is 2.931 us / 3.006 us, showing no history-length traversal. Prepared weight lookup is approximately 30-40 ns for Iceberg/Paimon.