branch-4.1: [feat](cache-memory) add external metadata cache memory governance - #66717
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
…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 |
…minal lookup outcomes The planning fence resolved the catalog's current authenticator eagerly even for projections that carry no captured context, so scan-node rigs without a stubbed catalog hit an NPE while the fence itself would have passed trivially; the current context is now resolved only when a captured context exists to compare against. testEntryFailsFastAfterCatalogRemoved depended on whichever Env the surefire fork happened to carry to pick between the uninitialized and dropped terminal messages; it now pins both outcomes through the dropped-catalog probe: a still-registered catalog fails as uninitialized, a dropped id fails as dropped.
|
run buildall |
…erging branch-4.1 Base commit a614930 routes the Doris table-cache miss loader through PaimonExternalCatalog.reloadPaimonTable (invalidating Paimon's CachingCatalog entry before loading). Rigs that exercise the miss loader stubbed the old getPaimonTable entry point and received null tables in the merged tree; they now stub and verify reloadPaimonTable, and the retained-handle schema test asserts neither entry point is consulted.
|
run buildall |
|
run feut |
1 similar comment
|
run feut |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4 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.