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 12 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
There was a problem hiding this comment.
Requesting changes for two cache-governance defects:
- Iceberg over-reserves wide merged partition ranges for endpoint objects that are discarded, so fitting snapshot metadata can be rejected or useful entries evicted.
- Paimon retains generation-keyed snapshot/schema children when the base-table load is returned but rejected from the cache, allowing unreachable projections to accumulate.
Review checkpoints:
- Correctness and accounting: traced hierarchical reservation, admission, eviction, refresh/replacement, and dependent-generation ownership paths across the managed cache core and connectors.
- Lifecycle, configuration, and compatibility: reviewed catalog property initialization/ALTER/replay paths, Hive/Iceberg/Paimon invalidation behavior, and the FE/BE statistics boundary; no additional non-duplicate issue survived.
- Test coverage: reviewed the changed unit, calibration, benchmark, and regression surfaces. Both inline findings identify a missing production-shaped regression. No builds or tests were run, as required by the review environment.
- Existing context: deduplicated against all 40 current inline comments and 9 landed review bodies.
- User focus: no additional user-provided review focus was specified.
- Completion: converged after two full review rounds; every candidate is resolved and both accepted findings are included inline.
Reviewed head b8fcc6db151b2b25dbfcf6fc0cf2996a9bf58848 against the authoritative bundle for base 0e53b31f58674a4716f12cdab4c46f3cb607ed05.
| nameToPartition.put(partition.getPartitionName(), partition); | ||
| retainedPayloadBytes = MetaCacheWeightUtils.saturatedAdd( | ||
| retainedPayloadBytes, partition.getRetainedPayloadBytes()); | ||
| retainedPayloadBytes = MetaCacheWeightUtils.saturatedAdd( |
There was a problem hiding this comment.
[P2] Charge range width only for surviving merged items
This surcharge is added for every physical partition before mergeOverlapPartitions() mutates nameToPartitionItem and drops enclosed ranges. It models the two PartitionKey endpoints of each RangePartitionItem, so those deleted endpoint graphs are not retained; the physical IcebergPartition values/transforms are already charged separately. For a wide mixed-spec overlap group, the excess grows with every enclosed day/hour partition and can reject a snapshot that fits or evict useful metadata. Please apply the width surcharge after merging using nameToPartitionItem.size(), and add a width-greater-than-one fixture that actually removes enclosed ranges.
| ignored -> executeAuthenticated(nameMapping, | ||
| () -> latestSnapshotProjectionLoader.loadAtFence( | ||
| nameMapping, fence, tableValue.getGeneration()))); | ||
| PaimonTableCacheValue currentTable = tableEntry.get(nameMapping.getCtlId()).peekIfPresent(nameMapping); |
There was a problem hiding this comment.
[P2] Retire children when base admission is rejected
MetaCacheEntry intentionally returns a loaded value even when its table estimate is incomplete or exceeds meta.cache.paimon.table.max-weight, but this guard invalidates the child only when another table is present. In the rejection case peekIfPresent() remains null; each call receives a fresh synthetic table generation and leaves its snapshot plus schema entries under keys no future call can reuse. With only the table weight configured, those children remain count-bounded and can accumulate until TTL/capacity eviction while repeatedly reloading partitions. Please treat a null current table as stale in both post-load guards (or prevent dependent publication without an admitted base), and add a repeated-rejection regression that keeps snapshot/schema cardinality stable.
…and retire Paimon projections of unpublished tables - IcebergUtils.loadPartitionInfo charges the per-column range endpoint width once per Doris partition that survives mergeOverlapPartitions instead of once per physical partition; the range endpoint and partition constants are recalibrated against own-array fixtures - PaimonExternalMetaCache treats an absent (rejected, expired) base table generation as stale so snapshot/schema projections keyed by it never accumulate
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for three cache-lifecycle correctness issues found in the full authoritative diff:
- [P1] Iceberg same-metadata refresh can leave scans on the previous table handle's expired vended credentials and FileIO.
- [P2] Removing an admitted Paimon base entry does not retire its generation-keyed children.
- [P2] Rejecting an oversized Iceberg base entry can still publish unowned snapshot/schema children.
I reviewed the shared cache framework, catalog lifecycle and configuration, Hive/Paimon/Iceberg integrations, estimators, scan/write/system-table paths, rolling-upgrade surface, benchmarks, and tests. Existing review threads were used as a duplicate fence; the analogous Paimon rejected-admission issue and previously raised generation, telemetry, and compatibility issues are not repeated. The user focus file contained no additional focus points. Per the review task, no builds or tests were run.
|
|
||
| private void retireTableGeneration(NameMapping nameMapping, | ||
| @Nullable IcebergTableCacheValue previousValue, IcebergTableCacheValue currentValue) { | ||
| if (previousValue != null && previousValue.isSamePhysicalGeneration(currentValue)) { |
There was a problem hiding this comment.
[P1] Refresh the snapshot handle when operational credentials change
This early return also preserves the old snapshot cache value when the catalog refresh returns the same UUID and metadata file with renewed operational resources. That value's FrozenTableOperations captures the previous FileIO, encryption manager, and location provider; IcebergScanNode later swaps to that frozen table and derives vended storage credentials from it. For catalogs that rotate short-lived credentials without publishing new metadata, subsequent scans can therefore keep using expired credentials even though the base table entry refreshed successfully. Please retire or rebind the operational snapshot value on table-handle refresh (schema projections can remain generation-keyed), and cover same-UUID/same-location refreshes with distinct credential-bearing FileIO instances.
| MetaCacheEntryInvalidation.forNameMapping(nameMapping -> nameMapping))); | ||
| MetaCacheEntryInvalidation.forNameMapping(nameMapping -> nameMapping)) | ||
| .withSizeEstimator((key, value) -> value.prepareForCachePublication(key)) | ||
| .withReplacementListener(this::retireTableGeneration)); |
There was a problem hiding this comment.
[P2] Retire children when an admitted base entry is removed
This replacement listener runs only when a new value is successfully published. Capacity eviction, expiry, soft-value collection, and peer reclaim remove an already-admitted table through the normal removal path without calling retireTableGeneration; a later load receives a fresh synthetic generation, so the old snapshot/schema keys are no longer addressable. This is the already-admitted removal path, distinct from the existing rejected-admission and successful-replacement threads. Because those child entries have independent limits, evicting cold base tables can leave their children consuming capacity and budget until the child TTL expires. Please add a generation-aware removal callback (fenced so a delayed callback cannot retire a successor), and test base capacity/expiry removal before any replacement is published.
| return value; | ||
| })); | ||
| IcebergTableCacheValue currentTable = tableEntry.get(nameMapping.getCtlId()).peekIfPresent(nameMapping); | ||
| if (currentTable != null && !tableValue.isSamePhysicalGeneration(currentTable)) { |
There was a problem hiding this comment.
[P2] Do not retain children for a rejected table generation
getWithManualLoad() returns the loaded table even when weighted admission rejects it, so this path can still publish snapshot and schema projections. In that case peekIfPresent() is null, but both post-load guards treat null as valid; no table replacement is published, so retireTableGeneration can never remove these children. The analogous Paimon site is already under review, but this separate Iceberg implementation still has the null-parent exemption. As metadata locations advance, repeated oversized tables can therefore leave multiple full table graphs in the count-bounded child caches despite meta.cache.iceberg.table.max-weight. Please treat an absent current base as stale (or prevent dependent publication unless base admission succeeds), and add a repeated-rejection test that advances metadata locations.
…e children of removed base tables - Iceberg: a table handle refresh that keeps the metadata generation but renews FileIO, encryption or location provider retires the snapshot projections frozen on the previous handle; post-load guards treat an absent (rejected) base table as stale - metacache: MetaCacheEntryRemovalListener delivers admitted values removed by eviction, expiry, collection or invalidation asynchronously; invalidateIf(BiPredicate) uses quiet lookups - Paimon: retire snapshot/schema projections keyed by a removed table generation, fenced by that generation
|
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. |
There was a problem hiding this comment.
Automated review completed over the authoritative 91-file diff after three full normal and risk-focused rounds.
Status: capped/incomplete. The third and final permitted round still produced a new finding, so I am not claiming full convergence; all five substantiated findings are included inline.
The actionable issues are:
- removal-listener handoff can retain weighted values after accounting is released;
- Paimon latest-snapshot churn retains obsolete same-generation projections;
- an ineffective Iceberg base can leave an active snapshot bound to expired operational credentials;
- an ineffective Paimon base leaves active child caches permanently cold;
- ineffective but weight-bounded Iceberg entries still repeat publication-sizing work that cannot be admitted.
Critical checkpoints covered: cache admission/removal/refresh/configuration/replay and budget ownership; Hive/Paimon/Iceberg generation and estimator lifetimes; Iceberg query isolation and writable rebinding; FE/BE statistics compatibility; and changed tests, regressions, benchmark/POM wiring, and unchanged call-site assumptions. Existing live threads were treated as duplicate fences.
No additional user review focus was supplied. Per the runner contract, builds and tests were not executed.
| return; | ||
| } | ||
| if (removalListener != null) { | ||
| pendingRemovalNotifications.add(new RemovedValue<>(key, value)); |
There was a problem hiding this comment.
[P1] Keep queued removal values inside the memory budget
pendingRemovalNotifications now strongly owns the entire removed V, but removals performed under admissionLock release that value's reservation immediately below. invalidateAll/invalidateIf and local/peer eviction can therefore admit replacements while the single process-wide cleanup thread still holds retired Paimon table graphs; each callback also scans snapshot/schema entries, so remove/refill churn can build an unbounded queue outside entry/catalog/global accounting. Please either keep the reservation until the notification drops V or enqueue only the generation/token needed by the listener, and test a blocked cleanup plus repeated invalidate/refill cycle.
| return tableEntry.get(nameMapping.getCtlId()).get(nameMapping).getLatestSnapshotCacheValue(); | ||
| PaimonTableCacheValue tableValue = tableEntry.get(nameMapping.getCtlId()).get(nameMapping); | ||
| PaimonSnapshot fence = loadLatestSnapshotFence(nameMapping, tableValue.getPaimonTable()).getSnapshot(); | ||
| PaimonSnapshotEntryKey key = PaimonSnapshotEntryKey.of( |
There was a problem hiding this comment.
[P2] Retire superseded snapshots within the current table generation
This method re-reads the latest fence on every call and includes its snapshot/schema IDs in the key, but retireTableGeneration only removes keys from other synthetic table generations. Each commit observed before tableEntry refresh thus publishes a new full partition/table projection while earlier same-generation values can never be looked up again; they remain charged until the next successful base refresh, 24-hour access expiry, or capacity/weight eviction, so a busy table can displace or reject current metadata. Please race-safely retain only the newest latest key per (NameMapping, tableGeneration), and test advancing IDs without replacing tableValue, including reversed concurrent completion.
| })); | ||
| MetaCacheEntry<NameMapping, IcebergTableCacheValue> tables = tableEntry.get(nameMapping.getCtlId()); | ||
| IcebergTableCacheValue currentTable = tables.peekIfPresent(nameMapping); | ||
| if (tables.isEffectivelyEnabled() |
There was a problem hiding this comment.
[P1] Revalidate snapshot resources when the base is ineffective
A valid meta.cache.iceberg.table.max-weight=0 makes the base entry ineffective while leaving snapshot caching enabled by default; table.enable=false with an explicit snapshot.enable=true reaches the same state. The table lookup then returns a fresh uncached handle on every call, but this guard skips all checks and a same-physical-key snapshot hit keeps the old FrozenTableOperations/FileIO; no base replacement listener can retire it, so regularly accessed scans can reuse expired vended credentials indefinitely. Please compare the fresh tableValue with retained snapshot resources whenever the base is ineffective (or disable that child state), and test same-metadata credential rotation under both configurations.
| * so its projections must not stay behind in the child entries. | ||
| */ | ||
| private boolean isCurrentTableGeneration(NameMapping nameMapping, long tableGeneration) { | ||
| PaimonTableCacheValue currentTable = tableEntry.get(nameMapping.getCtlId()).peekIfPresent(nameMapping); |
There was a problem hiding this comment.
[P2] Do not leave child caches permanently cold when the base is ineffective
A valid meta.cache.paimon.table.max-weight=0 makes the base ineffective while leaving both children enabled by default; table.enable=false also leaves schema enabled and can explicitly re-enable snapshots. Every table lookup then returns an uncached value with a fresh synthetic generation, so this check always fails and immediately discards the just-loaded child. Repeated schema calls reload the table/schema, while snapshot calls repeat fence discovery and full partition enumeration even though those caches report enabled. Please either effectively disable/reject dependent children when the base is ineffective, or give these loads a safe reusable physical identity, and cover repeated schema/snapshot lookups under these configurations.
| IcebergTableCacheValue value = new IcebergTableCacheValue(table); | ||
| MetaCacheEntry<NameMapping, IcebergTableCacheValue> currentEntry = | ||
| tableEntry.getIfInitialized(nameMapping.getCtlId()); | ||
| if (currentEntry != null && currentEntry.isWeightBounded()) { |
There was a problem hiding this comment.
[P2] Skip publication sizing when the entry is ineffective
An entry can be ineffective yet remain isWeightBounded() (for example max-weight=0, or enable=false/zero TTL/capacity combined with a direct, catalog, or global weight). This check therefore runs full prepareTableForCachePublication() on every uncached lookup even though MetaCacheEntry cannot admit the value, repeatedly walking/serializing the retained metadata graph solely for governance that can never produce a hit. The snapshot path at lines 187/196 and manifest accounting flag at line 272 use the same mismatched predicate. Please require both weight-bounded and effectively enabled for publication preparation/accounting, with ineffective weighted table/snapshot/manifest regressions.
…n ineffective bases and keep one Paimon latest projection per generation - metacache: removal listeners receive a token extracted at removal time so retired values never wait outside the memory budget; isWeightAccounting() gates publication sizing so ineffective weighted entries skip preparation and query isolation - Iceberg: snapshot hits are revalidated against the fresh table handle's operational resources whenever the base entry publishes nothing (max-weight 0, table cache disabled) - Paimon: bypass generation-keyed child caches when the base entry is ineffective; after a load keep only the most recently observed latest-fence projection of a table generation (covers reversed completion and rollback)
|
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. |
|
/review |
There was a problem hiding this comment.
Review completed and converged after three full normal and risk-focused rounds over the authoritative 91-file diff at 29ffdd681c1a7317aaab8462c549c1402a8d70c7. Existing review threads and the current live comments were deduplicated, and no candidate remains unresolved.
I found four P2 issues that should be addressed before merge:
- Rejected Paimon table generations retain unbounded latest-fence owner records outside the cache budgets.
- Paimon partition accounting charges shared schema-name strings once per partition and can falsely reject otherwise admissible projections.
- Catalog retirement holds shared engine/striped lifecycle locks through a full cache-group close, blocking unrelated catalog initialization.
- Peer reclamation sorts all active entry budgets while holding the FE-wide accounting lock, pausing unrelated admissions and releases under pressure.
Checkpoint results:
- Cache admission, ownership, removal, refresh, and lifecycle paths were traced across the manager, budget, entry, and connector layers. The two lifecycle/accounting issues above survived concurrency and fix-validity challenge; the other suspicious paths were either disproved or already covered by live threads.
- Hive, Iceberg, and Paimon identity, estimator, query-snapshot, writer/action, and invalidation paths were reviewed. Beyond the two Paimon findings above, no distinct unthreaded connector defect survived.
- FE/BE statistics-schema compatibility, property validation/replay, and rolling-policy paths were checked. Remaining concerns in those areas are already represented by existing review threads.
- Changed unit, benchmark, and regression coverage was reviewed. The missing concurrency/cardinality cases are called out inline. Builds and tests were not run because the authoritative review instructions explicitly prohibit them.
- User focus: no additional focus was provided, so the full PR scope received the normal review.
Requesting changes for the four inline issues.
| nameMapping, fence, tableValue.getGeneration()); | ||
| })); | ||
| LatestFenceOwner owner = new LatestFenceOwner(nameMapping, tableValue.getGeneration()); | ||
| ObservedFence latest = latestObservedFences.compute(owner, (ignored, current) -> |
There was a problem hiding this comment.
[P2] Remove owners for generations that were never published
tables.get() still returns the loaded table when weighted admission rejects it, and every such load has a fresh synthetic generation. This compute therefore retains a distinct owner on each lookup; the later current-generation check only invalidates the snapshot child, while a rejected generation gets neither a replacement nor a removal callback that could retire the owner. The same ordering can resurrect an old owner when a blocked load resumes after replacement or catalog invalidation already performed cleanup. Persistently oversized or unsupported tables can thus grow latestObservedFences without any cache/budget bound. Please conditionally remove the published (owner, latest) when the generation is not current, and cover both repeated rejection and a delayed old-generation load with an owner-cardinality assertion.
| String partitionValue = typedSpec.get(partitionColumnName); | ||
| partitionValues.add(partitionValue); | ||
| orderedTypedSpec.put(partitionColumnName, partitionValue); | ||
| retainedPayloadBytes = PaimonPartitionInfo.addRetainedStringPayload( |
There was a problem hiding this comment.
[P2] Count shared partition-column names once
partitionColumnName is the stable string held by the schema Column, and that same reference is inserted into every partition's orderedTypedSpec. Accumulating its full string payload inside the outer partition loop therefore charges N copies even though the retained graph has one string plus N map references (whose structural cost is already charged separately). Large partition sets can exceed an entry/catalog limit solely because of this overcount and remain permanently cold. The current JOL/benchmark fixtures hide the mismatch by constructing fresh field-name strings per partition. Please charge these schema names once outside the loop (while retaining per-partition entry costs) and calibrate with shared production-style name identities.
|
|
||
| @Override | ||
| public void invalidateCatalog(long catalogId) { | ||
| public synchronized void invalidateCatalog(long catalogId) { |
There was a problem hiding this comment.
[P2] Do not close a catalog while holding shared lifecycle locks
removed.close() synchronously invalidates every entry and releases every reservation in the catalog, but it runs under both the engine-wide AbstractExternalMetaCache monitor and the manager's lifecycle stripe. There are only 64 stripes, so dropping or changing one large catalog blocks first use/reinitialization of every catalog for that engine and also unrelated, possibly different-engine catalogs whose IDs collide on the stripe. Please replace the shared stripe with genuinely per-catalog lifecycle state, detach under the engine monitor, then close after releasing the engine-global lock while retaining only that catalog's fence; alternatively make budget ownership generation-aware before allowing old/new groups to coexist. A test can block one close and verify progress for a second same-engine ID and a stripe-colliding ID.
| return; | ||
| } | ||
| List<EntryBudget> candidates; | ||
| synchronized (lock) { |
There was a problem hiding this comment.
[P2] Sort reclaim candidates outside the global budget lock
drainPeerReclaim holds the one FE-wide accounting lock while filtering and sorting every active EntryBudget. That same lock gates every reserve, resize, release, budget create/close, and stats operation, so with many initialized catalogs a pressure-triggered reclaim becomes an O(E log E) global pause for unrelated cache traffic. Please snapshot the candidate/order fields under the lock, sort outside it, revalidate current/closed state, and recompute the current deficit before invoking each reclaimer. A many-budget concurrency test should pause candidate ordering and prove an unrelated reserve/release can still complete.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. 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: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
…atest-fence owners of unpublished generations - Partition column names are schema-owned strings shared by every partition's typed spec; charge each distinct reference once so wide partition sets are not overcounted out of the weight budget, and calibrate fixtures with production-style shared name identities - Remove the latest-fence owner registered by a lookup whose table generation was never published (rejected admission, replaced or invalidated mid-load) so persistently rejected tables cannot grow the owner map
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review summary
I reviewed the complete authoritative 91-path change set and deduplicated against every existing inline thread. Two additional P2 issues remain:
- Catalog retirement repeatedly scans every weighted-entry scope while holding the FE-wide accounting lock.
- Empty Hive partitioned tables omit the retained partition-type key width from admission accounting.
Critical checkpoints:
- Correctness, lifecycle, and concurrency: catalog preparation/publication, reservation ownership, invalidation, close, soft collection, refresh, and peer reclaim were traced. No additional non-duplicate issue survived beyond the first inline comment and issues already present in live threads.
- Compatibility and configuration: the FE/BE statistics column contract, old-FE prefix retry, nullable padding, CREATE/ALTER validation, replay sanitization, and local-global limit clamping were checked. The remaining inverse rolling-upgrade concern is already covered by an existing thread.
- Connector boundaries: Hive event copy-on-write, all Iceberg retained/query/writable table callers and authentication scopes, and Paimon generation/fence ownership were reviewed. No further distinct snapshot, mutation, authentication, or owner-lifecycle issue survived deduplication.
- Accounting and coverage: Hive, Iceberg, and Paimon retained-graph formulas, calibration tests, benchmarks, unit tests, and regression additions were reviewed. The second inline comment identifies the remaining distinct Hive accounting gap.
- User focus: no additional review focus was provided.
No builds were run, as required by the review environment contract.
| entryBuckets.remove(entryBudget.scope, entryBudget.entryBucket); | ||
| entryBudgets.remove(entryBudget.scope, entryBudget); | ||
| Bucket catalogBucket = entryBudget.catalogBucket; | ||
| boolean catalogStillReferenced = entryBuckets.keySet().stream() |
There was a problem hiding this comment.
[P2] Avoid scanning every budget while holding the global lock
This close path runs once for each weighted entry, but while holding the FE-wide accounting lock it traverses every remaining entry scope to discover whether this catalog still has an entry. Dropping or reconfiguring one catalog therefore performs repeated O(total weighted entries) scans in the critical section, blocking unrelated reservation, release, statistics, creation, and close traffic across the FE. This remains after fixing the separate lifecycle-close and reclaim-sort threads. Please keep a per-catalog live-entry count in the bucket so this decision is O(1), and cover unrelated reservation progress during a many-catalog close.
| MetaCacheWeightUtils.saturatedMultiply( | ||
| value.getPartitionColumnCount(), PARTITION_COLUMN_BYTES)); | ||
| long bytes = MetaCacheWeightUtils.saturatedAdd( | ||
| ENTRY_BASE_BYTES, MetaCacheWeightUtils.estimatedNameMappingBytes(key.getNameMapping())); |
There was a problem hiding this comment.
[P2] Account for the retained key width on empty tables
PartitionValueCacheKey retains an immutable copy of types, but this formula only charges partition-column width inside partitionCount * perPartitionBytes. For an empty partitioned table, partitionCount is zero, so a key with one type and one with many types receive the same estimate even though the immutable list and backing references retained by Caffeine grow with width. Charge the key list and slots independently of partition count, and add an empty-table narrow-versus-wide calibration rooted at both the key and value.
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.