Skip to content

[refactor](fe) Refactor external metadata cache with MetaCacheEntry#65126

Open
wenzhenghu wants to merge 46 commits into
apache:masterfrom
HYDCP:wzh/external-meta-cache-refactor
Open

[refactor](fe) Refactor external metadata cache with MetaCacheEntry#65126
wenzhenghu wants to merge 46 commits into
apache:masterfrom
HYDCP:wzh/external-meta-cache-refactor

Conversation

@wenzhenghu

@wenzhenghu wenzhenghu commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: N/A

Related PR: #64705

Problem Summary:

This PR refactors the FE external metadata cache from the old MetaCache<T> wrapper into independent MetaCacheEntry instances and introduces NameCacheValue as the immutable names snapshot used by catalog/database name resolution.

The refactor keeps slow remote I/O out of Caffeine's synchronous load path, makes publication/invalidation generation-aware, and unifies the lower-case mode 0/1/2 behavior around one names snapshot.

After the initial refactor, PMC review feedback identified several follow-up items. This branch now incorporates those review-driven updates:

  1. Fix the mode-2 replay corner case when a names-only invalidation makes the names snapshot cold while the object cache is still hot. Replay remains cache-only and now falls back to a case-insensitive scan over hot object-cache keys instead of reloading names.
  2. Optimize NameCacheValue hot-path lookups by precomputing local-name indexes and exposing a cached localNames() view, while keeping names() defensive because Pair is mutable.
  3. Add comments that clarify the intended semantics for same-key load deduplication, hot-entry-only incremental names maintenance, generation-aware invalidation, and mode-2 cache-key conventions.

Some review findings were intentionally not turned into extra logic changes in this PR:

  • The invalidate generation flow is kept as-is because the all-stripe bump and the per-key invalidation path protect different publication windows.
  • Hot-entry-only incremental names updates are kept as-is because the observed race leads to an extra reload, not incorrect metadata state.
  • The repeated catalog/database code is intentionally not extracted into a shared helper in this follow-up patch. Although the shapes are similar, the two paths still differ in replay semantics, lower-case handling, object/ID bookkeeping, and test scaffolding. For this review-driven update, extracting a helper would widen the refactor surface and increase regression risk.

Implementation details

1. Overall architecture

The legacy cache grouped the names cache, object cache, and ID-to-name index in one MetaCache<T> wrapper:

ExternalCatalog / ExternalDatabase
    -> MetaCache<T>
        -> namesCache: LoadingCache<String, List<Pair<remote, local>>>
        -> metaObjCache: LoadingCache<String, Optional<T>>
        -> idToName

The refactored structure makes each logical dataset an independent MetaCacheEntry:

ExternalCatalog
    -> databaseNames: MetaCacheEntry<String, NameCacheValue>
    -> databases:     MetaCacheEntry<String, ExternalDatabase>
    -> dbIdToName

ExternalDatabase
    -> tableNames: MetaCacheEntry<String, NameCacheValue>
    -> tables:     MetaCacheEntry<String, ExternalTable>
    -> tableIdToName

The main architectural changes are:

  • Remove the MetaCache<T> wrapper that owned two LoadingCache instances.
  • Use one MetaCacheEntry<K, V> for each logical cache dataset.
  • Store the names list and its case-insensitive lookup index in one immutable NameCacheValue snapshot.
  • Move slow miss loading out of Caffeine's synchronous load path. Striped load locks deduplicate loads, while publication locks and generations coordinate explicit mutations, invalidation, and stale-result suppression.

2. Configuration

Config.external_meta_cache_object_entry_lock_stripes is a non-mutable configuration with a default value of 256. It controls the number of load-lock, publication-lock, and generation stripes used by multi-key MetaCacheEntry instances. Keys mapped to the same stripe share the same load and publication locks, so the value balances memory usage against unrelated-key contention.

Config.enable_external_meta_cache_name_miss_refresh is mutable and defaults to true. It controls what happens when a names snapshot is already cached but a requested database or table name is absent:

  • When enabled, Doris invalidates the names entry, synchronously reloads it once, and retries the lookup.
  • When disabled, the lookup returns not found without enumerating remote names again.

The option is enabled by default to preserve the existing external catalog visibility behavior. A database or table created outside Doris can therefore become visible on the next direct lookup without requiring an explicit refresh. Operators can disable the option to avoid repeated expensive listDatabaseNames() or listTableNames() calls when non-existent objects are queried frequently; with the option disabled, externally created objects become visible through an explicit refresh or metadata synchronization.

The previous enable_external_meta_cache_manual_miss_load switch is removed. MetaCacheEntry.get() now always uses manual miss loading and can no longer fall back to LoadingCache.get() for synchronous loader execution.

3. MetaCacheEntry

3.1 Concurrency state

MetaCacheEntry replaces the fixed 128 load-lock stripes and single global invalidation generation with configurable per-stripe state:

  • loadLocks deduplicate slow miss loads. Slow external I/O runs while holding only the corresponding load stripe, not a Caffeine bin lock or publication lock.
  • publishLocks protect short cache publication, explicit mutation, and key invalidation windows.
  • generations stores one generation per stripe so mutations to one stripe do not invalidate in-flight work on every other stripe.
3.2 Construction

The existing common constructors remain available, and overloads accepting an explicit stripeCount are added. All public constructors converge on one private constructor that validates the supported combinations and builds the underlying Caffeine cache.

withSyncRemovalListener(...) creates an entry with a synchronous removal listener. This path forces autoRefresh=false, because the existing synchronous-removal-listener cache construction must not be combined with refreshAfterWrite. Contextual-only entries must continue to use a null default loader and cannot enable automatic refresh.

All entries now use the custom CacheLoader returned by newCacheLoader() so that refreshAfterWrite reloads participate in generation checks.

3.3 Read APIs

get(K) always calls getWithManualLoad() with the default loader. It no longer reads a runtime feature switch and never invokes loadingData.get(key) for a normal miss.

get(K, Function<K, V>) uses the same manual path but executes the contextual loader supplied by the caller.

getIfPresent(K) never performs synchronous load-through. It returns null when the entry is disabled. A cache hit may still cause Caffeine to schedule asynchronous refreshAfterWrite, but the caller never waits for remote I/O.

3.4 Explicit mutations

put(K, V) rejects null keys and values and is a no-op when caching is disabled. Otherwise, it acquires the key's publication lock, increments the stripe generation, and writes the value. Incrementing the generation prevents an earlier slow load from later overwriting the explicit value.

compute(K, BiFunction<K, V, V>) is the generation-aware replacement for upper-layer direct access to data.asMap().compute(). It performs the generation bump and map mutation under the publication lock. As with ConcurrentMap.compute, a null remapping result removes the key. When caching is disabled, the remapping function is not executed.

3.5 Invalidation

invalidateKey(K) acquires the publication lock, bumps the key stripe generation, removes the key, and increments the invalidation count only when a cached value was actually removed.

invalidateIf(Predicate<K>) first calls bumpAllGenerations() so that in-flight loads whose keys have not yet appeared in the cache map cannot publish after the invalidation. It then iterates visible keys and delegates matching keys to invalidateKey(). The initial all-stripe bump is intentionally conservative: an absent in-flight key cannot be evaluated against the predicate without additional tracking.

invalidateAll() also bumps every stripe before iterating and invalidating each visible key. It no longer calls Caffeine's invalidateAll() directly, ensuring that published keys pass through the same publication coordination as single-key invalidation.

3.6 Manual miss loading

getWithManualLoad() implements the following sequence:

  1. If caching is disabled, execute and track the loader without writing the result into the cache.
  2. Check getIfPresent() on the fast path.
  3. On a miss, acquire the key's load stripe and recheck the cache to deduplicate slow loads.
  4. Briefly acquire the publication lock, recheck the cache again, and snapshot the stripe generation. This prevents an explicit mutation from occurring between miss observation and version capture.
  5. Execute the slow loader outside the publication lock and outside Caffeine's synchronous load path.
  6. Leave null loader results uncached.
  7. Reacquire the publication lock. If the generation changed, return the loaded value to the current caller but do not cache it.
  8. If the generation is unchanged, publish through putLoadedValueWithoutGenerationBump() and retain a post-put generation check.

putLoadedValueWithoutGenerationBump() is deliberately separate from the public put(): an internal loader publication must not invalidate the generation it captured for itself.

removeLoadedValueWithoutGenerationBump() removes the value only when the currently cached object is the same object published by that load. This avoids deleting a newer explicit replacement.

3.7 Generation-aware asynchronous refresh

newCacheLoader() keeps load() for Caffeine refresh support but normal misses no longer use it. Its asyncReload() implementation:

  1. Captures the key stripe generation before submitting the loader.
  2. Runs the loader on the configured executor.
  3. On completion, enters the publication lock and compares the current generation.
  4. Completes the reload future when the generation is unchanged, allowing Caffeine to publish the refresh result.
  5. Cancels the future when the generation changed, discarding a stale refresh result.
3.8 Stripe helpers

stripe(K) maps a key hash to a bounded stripe. loadLock(K) and publishLock(K) return the corresponding lock objects. generationOf(K) reads the stripe generation, bumpGeneration(K) invalidates work on one stripe, and bumpAllGenerations() invalidates publication eligibility across all stripes, including loads not yet represented in the cache map.

defaultObjectStripeCount() reads the configured multi-key stripe count. singleKeyStripeCount() returns one for names entries, whose only key is the empty string.

4. NameCacheValue

NameCacheValue is a new immutable snapshot containing both:

  • The ordered list of (remoteName, localName) pairs.
  • A lower-case name -> original remote name index for case-insensitive lookup.

The constructor deep-copies every Pair, builds the index with Locale.ROOT, and uses last-write-wins behavior for the index. Catalog-aware loaders remain responsible for detecting unsupported case conflicts.

of(...) creates a non-null snapshot, and empty() creates an empty snapshot for explicit forced-update paths. names() returns fresh Pair objects so callers cannot mutate a published snapshot through the mutable Pair type.

remoteNameOfLocalName() replaces the legacy MetaCache.getRemoteName() lookup and now uses a precomputed local-name index. containsLocalName() supports existence checks in object initialization and now uses a precomputed local-name set. localNames() exposes a cached immutable local-name view for hot read paths like SHOW DATABASES and SHOW TABLES. remoteNameForCaseInsensitiveLookup() replaces the separate lowerCaseToDatabaseName and lowerCaseToTableName maps.

withName(remoteName, localName) performs copy-on-write insertion. It rejects an exact remote name already mapped to a different local name, removes any old entry for the same local name, adds the new pair, and returns a new snapshot. Callers update this names snapshot before object and ID state so a conflict cannot leave a partially updated object cache.

withoutLocalName(localName) performs copy-on-write removal and returns a new snapshot. copyPairs() provides the deep-copy boundary used during construction and readout.

5. ExternalCatalog

5.1 Cache structure and initialization

The legacy MetaCache<ExternalDatabase> and lowerCaseToDatabaseName side map are replaced by databaseNames, databases, and dbIdToName.

buildMetaCache() creates:

  • databaseNames, keyed only by "", with a NameCacheValue, capacity one, one stripe, and refreshAfterWrite enabled.
  • databases, keyed by local database name, with configurable object stripes, refreshAfterWrite disabled, and a synchronous removal listener that calls resetMetaToUninitialized() on the removed database object.

getFilteredDatabaseNames() still performs remote enumeration, include/exclude filtering, remote-to-local mapping, system database handling, and case-conflict validation. It no longer mutates a separate lower-case side map and now acts as the pure loader for a complete names snapshot.

buildDbForInit() routes existence checks through resolveDatabaseNameFromSnapshot() and routes local-to-remote resolution through getRemoteDatabaseName(). The system database construction branches and the existing unit-test runningUnitTest bypass condition are preserved.

5.2 Normal and replay lookup

getDbNames() reads local database names from databaseNames instead of MetaCache.listNames().

getDbNullable(String) preserves information_schema and mysql name normalization. Other names are resolved to a local name before databases.get() performs manual miss loading. A successful lookup updates dbIdToName using db.getId() as the single ID source.

getDbNullable(long) resolves the local name through dbIdToName and then calls databases.get(). It returns null immediately when no ID mapping exists.

getDbForReplay(long) checks initialization, resolves the key through dbIdToName, and uses databases.getIfPresent(). This fixes the legacy path where getMetaObjById() could perform load-through during replay.

getDbForReplay(String) first checks the exact object key. On a miss, it resolves the local name only from an existing names snapshot and performs another getIfPresent(). If mode 2 replay sees a cold names snapshot but the object cache is still hot, it now performs a cache-only case-insensitive fallback over hot object-cache keys instead of reloading names. Replay misses never perform synchronous remote loading. A hot names entry may still schedule background refreshAfterWrite, but the replay thread does not wait for it.

5.3 Name resolution

getLocalDatabaseName() preserves modes 0 and 1. Mode 2 now resolves the original-case name through NameCacheValue.remoteNameForCaseInsensitiveLookup().

getDatabaseNamesValue(boolean allowLoad) centralizes the choice between databaseNames.get("") and databaseNames.getIfPresent("").

resolveDatabaseNameFromSnapshot() centralizes negative-lookup behavior:

  • A cold non-replay miss loads the names entry normally.
  • A cold replay miss returns null without synchronous loading.
  • A hit returns the resolver result from the current snapshot.
  • A miss in an existing snapshot invalidates the names entry, reloads once, and retries by default. When enable_external_meta_cache_name_miss_refresh is disabled, it returns null without enumerating remote names again.

listLocalDatabaseNamesFromCache() loads the snapshot when necessary and extracts local names from the cached localNames() view. getRemoteDatabaseName() uses the shared resolver and remoteNameOfLocalName() to replace MetaCache.getRemoteName().

5.4 Incremental updates and invalidation

updateDatabaseCache() updates state in names -> object -> ID order. Runtime incremental events update names and object entries only when those entries are already hot: a single create event must not materialize an incomplete names snapshot or preheat an object the FE has never consumed. The lightweight dbIdToName navigation index is always updated from db.getId(), so normal by-ID lookup can load a cold object on demand. The event path and cold object loader use the same deterministic ID rule, so the reconstructed object retains the event ID. Replay by-ID lookup remains cache-only.

invalidateDatabaseCache() removes the local name through copy-on-write only when the names snapshot is already hot, invalidates the object key, and removes every ID mapping whose value equals that local name. Drop events therefore do not load a cold names entry.

invalidateDatabaseNamesOnly() invalidates only the names entry. invalidateAllDatabaseCache() invalidates names and objects and clears the ID index. refreshMetaCacheOnly() now delegates to the full helper.

unregisterDatabase() delegates local state cleanup to invalidateDatabaseCache() while preserving the following engine-level ExternalMetaCacheMgr.invalidateDb() call.

resetMetaCacheNames() preserves the existing CREATE DATABASE behavior by invalidating only the names entry.

resetToUninitialized() no longer clears a separate lower-case map. gsonPostProcess() reconstructs an empty dbIdToName, making the ID map explicitly runtime-only rather than image-persisted state.

6. ExternalDatabase

ExternalDatabase applies the same structure at table scope. MetaCache<T> and lowerCaseToTableName are replaced by tableNames, tables, and tableIdToName.

buildMetaCache() creates a single-key, auto-refreshing tableNames entry and a multi-key tables entry with autoRefresh=false. The table object entry does not need the database-level removal callback used by ExternalCatalog.databases.

listTableNames() preserves system database branches, remote enumeration, remote-to-local mapping, and conflict checks, but no longer mutates a lower-case side map.

resetMetaToUninitialized() delegates to invalidateAllTableCache() so names, objects, and IDs are cleared together before engine-level database cache invalidation.

buildTableForInit() uses resolveTableNameFromSnapshot() for existence checks and getRemoteTableName() for local-to-remote resolution. The original outer condition deciding whether remote-name mapping is necessary and the unit-test bypass condition are preserved.

getTableNamesWithLock() extracts local names from NameCacheValue. getTableNullable(String) resolves the local name, performs tables.get() manual loading, and updates tableIdToName from table.getId(). getTableNullable(long) resolves the local name from the ID index and then calls tables.get().

Both getTableForReplay(long) and getTableForReplay(String) require initialized state and use getIfPresent() for the final object lookup. The string overload checks the exact key before consulting the cached names snapshot. If mode 2 replay sees a cold names snapshot but the object cache is still hot, it now falls back to a cache-only case-insensitive scan over hot object-cache keys. Replay misses do not synchronously enumerate remote tables or load table objects.

isTableExist() no longer calls listTableNames() directly to populate a side map. For case-insensitive names, it resolves the remote name through the shared names resolver and then calls the catalog's tableExist() implementation.

getLocalTableName() preserves the existing lower-case storage modes and uses NameCacheValue for case-insensitive original-name resolution. getTableNamesValue(), resolveTableNameFromSnapshot(), listLocalTableNamesFromCache(), and getRemoteTableName() are the table-level counterparts of the database-name helpers described above.

updateTableCache() uses the same split semantics as the database side: names and object entries are updated only when already hot, while tableIdToName is always updated from the actual table ID. Normal getTableNullable(long) can therefore resolve the name and load a cold table object on demand. The event path and loader share the same deterministic table-ID rule; Replay still uses getIfPresent().

invalidateTableCache() removes the name from a hot snapshot, invalidates the object key, and removes matching ID mappings. unregisterTable() derives the local cache key with the same remote-to-local rules used by register/list and performs this local cleanup even when the object entry is cold or already evicted. Engine-specific cache invalidation is invoked only when the cached ExternalTable object is available. invalidateAllTableCache() clears all three structures.

registerTable() and unregisterTable() now delegate to these centralized update/invalidation helpers while preserving their surrounding timestamp behavior. resetMetaCacheNames() still invalidates only table names. gsonPostProcess() reconstructs an empty runtime-only tableIdToName map.

7. ExternalMetaCacheMgr

The LegacyMetaCacheFactory field, its construction, and the legacyMetaCacheFactory() accessor are removed because catalog and database objects now build MetaCacheEntry instances directly.

commonRefreshExecutor() exposes the existing common refresh executor to those construction paths. It replaces the old factory accessor without creating a new thread pool or changing executor ownership.

8. HMSExternalCatalog

registerDatabase(long, String) now delegates cache maintenance to ExternalCatalog.updateDatabaseCache() instead of calling metaCache.updateCache() directly. This centralizes names/object/ID ordering, hot-only names/object updates, always-on ID navigation maintenance, and the single ID source. The duplicate Util.genIdByName() call is removed because the helper uses db.getId().

9. AbstractExternalMetaCache

newMetaCacheEntry() now passes MetaCacheEntry.defaultObjectStripeCount() explicitly when constructing existing schema, partition, and other engine-specific entries. The new stripe configuration therefore applies consistently both to the database/table object caches migrated in this PR and to multi-key entries managed by this class.

Release note

Improve external metadata cache replay lookup and hot-path name resolution in FE.

Check List (For Author)

  • Test:
    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test (with reason)

Manual test details:

  • Validate mutable name miss refresh semantics on a real Doris instance with tools/manual_external_meta_cache_regression/verify_name_miss_refresh_mutable.sh
  • Validate tableNames refresh non-blocking on a real Doris instance with tools/manual_external_meta_cache_regression/verify_table_names_refresh_non_blocking.sh
  • Validate schema refresh non-blocking on a real Doris instance with tools/manual_external_meta_cache_regression/verify_schema_refresh_non_blocking.sh
  • Validate lower-case mode 0, 1 and 2 SQL paths on a real Doris instance

Unit test details:

  • ./run-fe-ut.sh --run org.apache.doris.datasource.metacache.MetaCacheEntryTest

  • ./run-fe-ut.sh --run org.apache.doris.datasource.metacache.NameCacheValueTest,org.apache.doris.datasource.ExternalDatabaseTest,org.apache.doris.datasource.ExternalCatalogTest

  • Behavior changed:

    • No
    • Yes. Replay lookup, names snapshot hot-path lookup, and review clarifications are aligned with the refactor design.
  • Does this need documentation:

    • No
    • Yes

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@wenzhenghu
wenzhenghu marked this pull request as draft July 1, 2026 14:20
@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

1 similar comment
@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29809 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 2231ed676f5c15ca4fc6f8f67ec2adf5b4923005, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17613	4059	4032	4032
q2	2036	315	193	193
q3	10284	1450	817	817
q4	4686	469	346	346
q5	7515	839	567	567
q6	179	166	140	140
q7	790	824	636	636
q8	9376	1623	1709	1623
q9	5571	4408	4432	4408
q10	6810	1768	1520	1520
q11	510	343	321	321
q12	702	545	430	430
q13	18138	3377	2757	2757
q14	269	255	238	238
q15	q16	784	776	704	704
q17	950	1000	999	999
q18	6800	5834	5606	5606
q19	1289	1325	1043	1043
q20	808	655	563	563
q21	5990	2720	2569	2569
q22	440	357	297	297
Total cold run time: 101540 ms
Total hot run time: 29809 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4365	4288	4296	4288
q2	287	319	208	208
q3	4665	4964	4485	4485
q4	2074	2145	1371	1371
q5	4451	4306	4342	4306
q6	234	180	129	129
q7	1753	1992	1833	1833
q8	2576	2195	2305	2195
q9	8043	8144	7763	7763
q10	4864	4773	4378	4378
q11	597	412	388	388
q12	766	772	564	564
q13	3207	3608	2953	2953
q14	295	313	287	287
q15	q16	706	738	668	668
q17	1346	1326	1480	1326
q18	7975	7314	7196	7196
q19	1192	1136	1079	1079
q20	2204	2181	1937	1937
q21	5244	4577	4401	4401
q22	561	465	409	409
Total cold run time: 57405 ms
Total hot run time: 52164 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 173774 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 2231ed676f5c15ca4fc6f8f67ec2adf5b4923005, data reload: false

query5	4335	647	495	495
query6	462	236	207	207
query7	4867	569	337	337
query8	339	188	169	169
query9	8747	4006	4024	4006
query10	508	344	306	306
query11	5961	2321	2169	2169
query12	164	103	101	101
query13	1260	598	459	459
query14	6271	5334	4986	4986
query14_1	4273	4307	4310	4307
query15	212	205	184	184
query16	1009	469	482	469
query17	1029	742	585	585
query18	2488	476	355	355
query19	214	195	163	163
query20	111	110	109	109
query21	242	157	132	132
query22	13665	13595	13441	13441
query23	17416	16531	16135	16135
query23_1	16299	16203	16245	16203
query24	7618	1749	1264	1264
query24_1	1340	1291	1293	1291
query25	578	460	395	395
query26	1341	338	212	212
query27	2619	551	385	385
query28	4517	2048	2018	2018
query29	1105	622	494	494
query30	336	264	228	228
query31	1123	1105	974	974
query32	143	66	63	63
query33	556	333	252	252
query34	1166	1197	663	663
query35	770	790	677	677
query36	1407	1418	1219	1219
query37	198	99	90	90
query38	1872	1704	1652	1652
query39	927	913	883	883
query39_1	884	883	889	883
query40	306	164	139	139
query41	69	63	64	63
query42	96	93	94	93
query43	327	320	284	284
query44	1446	776	760	760
query45	206	204	181	181
query46	1103	1185	714	714
query47	2396	2335	2253	2253
query48	428	406	299	299
query49	594	431	321	321
query50	1119	419	326	326
query51	4415	4501	4325	4325
query52	86	85	76	76
query53	265	276	208	208
query54	288	235	207	207
query55	76	70	67	67
query56	313	294	291	291
query57	1434	1414	1331	1331
query58	271	255	293	255
query59	1560	1683	1431	1431
query60	298	267	271	267
query61	157	155	170	155
query62	704	638	600	600
query63	248	202	211	202
query64	2528	767	604	604
query65	4869	4810	4751	4751
query66	1824	525	429	429
query67	29833	29581	29468	29468
query68	3382	1526	1038	1038
query69	422	308	271	271
query70	1069	973	975	973
query71	370	335	304	304
query72	2928	2647	2365	2365
query73	827	790	434	434
query74	5127	5040	4781	4781
query75	2618	2582	2250	2250
query76	2330	1179	788	788
query77	350	385	287	287
query78	12331	12345	11826	11826
query79	1449	1108	782	782
query80	762	557	471	471
query81	469	334	288	288
query82	878	158	121	121
query83	381	324	295	295
query84	325	168	140	140
query85	1003	621	535	535
query86	437	314	282	282
query87	1830	1821	1767	1767
query88	3717	2798	2764	2764
query89	472	409	364	364
query90	1927	205	198	198
query91	204	192	160	160
query92	64	58	59	58
query93	1554	1501	1014	1014
query94	624	349	316	316
query95	772	486	479	479
query96	1077	791	340	340
query97	2675	2702	2554	2554
query98	220	206	203	203
query99	1166	1144	1021	1021
Total cold run time: 259861 ms
Total hot run time: 173774 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 25.22 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 2231ed676f5c15ca4fc6f8f67ec2adf5b4923005, data reload: false

query1	0.01	0.01	0.01
query2	0.10	0.06	0.05
query3	0.26	0.13	0.13
query4	1.61	0.16	0.14
query5	0.24	0.23	0.22
query6	1.32	1.05	1.07
query7	0.04	0.00	0.00
query8	0.06	0.04	0.04
query9	0.38	0.30	0.32
query10	0.55	0.60	0.55
query11	0.20	0.15	0.14
query12	0.19	0.15	0.15
query13	0.47	0.48	0.47
query14	1.02	1.01	1.02
query15	0.60	0.60	0.59
query16	0.33	0.33	0.32
query17	1.08	1.09	1.16
query18	0.22	0.20	0.21
query19	2.01	1.92	1.89
query20	0.02	0.01	0.01
query21	15.44	0.22	0.15
query22	4.85	0.06	0.05
query23	16.13	0.33	0.12
query24	2.98	0.45	0.34
query25	0.13	0.05	0.04
query26	0.73	0.20	0.16
query27	0.07	0.04	0.03
query28	3.58	0.95	0.56
query29	12.46	4.31	3.43
query30	0.30	0.14	0.15
query31	2.77	0.63	0.31
query32	3.23	0.60	0.49
query33	3.26	3.18	3.19
query34	15.51	4.26	3.56
query35	3.54	3.54	3.60
query36	0.56	0.41	0.44
query37	0.10	0.07	0.06
query38	0.05	0.04	0.04
query39	0.04	0.02	0.03
query40	0.19	0.15	0.15
query41	0.11	0.04	0.03
query42	0.04	0.02	0.03
query43	0.04	0.04	0.03
Total cold run time: 96.82 s
Total hot run time: 25.22 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 81.87% (307/375) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 81.87% (307/375) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29829 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 6c7d1d903a6f137cbc1835cda462d23e54f4eb67, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17665	4099	4028	4028
q2	2029	319	203	203
q3	10275	1432	830	830
q4	4716	471	337	337
q5	7591	839	569	569
q6	184	170	137	137
q7	774	839	650	650
q8	10274	1626	1656	1626
q9	5943	4422	4452	4422
q10	6841	1824	1538	1538
q11	509	342	314	314
q12	744	565	433	433
q13	18087	3371	2734	2734
q14	266	269	241	241
q15	q16	790	780	703	703
q17	1010	1022	978	978
q18	6951	5851	5619	5619
q19	1181	1253	1075	1075
q20	788	638	597	597
q21	5705	2662	2493	2493
q22	439	366	302	302
Total cold run time: 102762 ms
Total hot run time: 29829 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4333	4318	4305	4305
q2	290	318	209	209
q3	4566	4993	4401	4401
q4	2073	2146	1397	1397
q5	4428	4304	4280	4280
q6	241	180	127	127
q7	2004	1936	1667	1667
q8	2453	2131	2072	2072
q9	7800	7769	7664	7664
q10	4738	4674	4272	4272
q11	563	424	382	382
q12	939	797	554	554
q13	3296	3522	3000	3000
q14	304	312	302	302
q15	q16	720	724	642	642
q17	1352	1322	1329	1322
q18	8043	7308	6880	6880
q19	1116	1089	1116	1089
q20	2186	2221	1960	1960
q21	5297	4633	4482	4482
q22	508	450	408	408
Total cold run time: 57250 ms
Total hot run time: 51415 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 173258 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 6c7d1d903a6f137cbc1835cda462d23e54f4eb67, data reload: false

query5	4316	639	504	504
query6	473	227	201	201
query7	4834	565	316	316
query8	331	188	181	181
query9	8775	4037	4003	4003
query10	434	357	294	294
query11	5881	2343	2163	2163
query12	157	106	108	106
query13	1265	638	428	428
query14	6301	5342	5002	5002
query14_1	4305	4290	4309	4290
query15	208	206	183	183
query16	1030	484	438	438
query17	1144	712	603	603
query18	2440	488	358	358
query19	208	194	157	157
query20	112	109	109	109
query21	231	172	150	150
query22	13721	13575	13312	13312
query23	17464	16517	16106	16106
query23_1	16406	16233	16290	16233
query24	7530	1765	1317	1317
query24_1	1336	1306	1293	1293
query25	583	463	412	412
query26	1336	371	221	221
query27	2568	596	377	377
query28	4494	2048	2041	2041
query29	1084	628	502	502
query30	333	263	230	230
query31	1116	1099	1042	1042
query32	99	59	58	58
query33	522	326	239	239
query34	1164	1173	653	653
query35	756	782	649	649
query36	1416	1385	1248	1248
query37	155	102	93	93
query38	1877	1690	1645	1645
query39	952	919	892	892
query39_1	878	878	903	878
query40	249	162	135	135
query41	69	67	64	64
query42	97	97	95	95
query43	324	323	288	288
query44	1412	764	761	761
query45	206	194	184	184
query46	1129	1187	747	747
query47	2378	2333	2165	2165
query48	422	392	271	271
query49	585	415	319	319
query50	1071	433	330	330
query51	4460	4477	4306	4306
query52	87	84	75	75
query53	260	273	214	214
query54	276	236	207	207
query55	74	71	71	71
query56	289	312	287	287
query57	1421	1416	1294	1294
query58	299	261	249	249
query59	1534	1604	1395	1395
query60	302	269	251	251
query61	158	143	149	143
query62	696	647	619	619
query63	247	209	198	198
query64	2518	777	631	631
query65	4873	4792	4734	4734
query66	1834	505	391	391
query67	29600	29533	29399	29399
query68	3167	1565	1056	1056
query69	408	291	268	268
query70	1083	973	962	962
query71	369	341	293	293
query72	2893	2581	2332	2332
query73	857	789	447	447
query74	5097	4943	4764	4764
query75	2611	2565	2225	2225
query76	2318	1192	781	781
query77	348	381	288	288
query78	12378	12499	11796	11796
query79	1388	1187	749	749
query80	828	527	462	462
query81	502	319	288	288
query82	569	162	131	131
query83	393	323	297	297
query84	279	165	130	130
query85	954	614	515	515
query86	396	309	297	297
query87	1842	1824	1730	1730
query88	3684	2767	2764	2764
query89	453	408	359	359
query90	1808	203	195	195
query91	201	217	163	163
query92	66	58	58	58
query93	1682	1595	977	977
query94	618	356	288	288
query95	778	497	491	491
query96	1087	840	347	347
query97	2676	2712	2617	2617
query98	217	206	199	199
query99	1181	1164	1034	1034
Total cold run time: 258682 ms
Total hot run time: 173258 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 25.27 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 6c7d1d903a6f137cbc1835cda462d23e54f4eb67, data reload: false

query1	0.01	0.01	0.01
query2	0.10	0.05	0.05
query3	0.26	0.15	0.13
query4	1.61	0.13	0.13
query5	0.24	0.23	0.23
query6	1.27	1.07	1.04
query7	0.04	0.01	0.01
query8	0.06	0.04	0.04
query9	0.37	0.31	0.33
query10	0.60	0.55	0.53
query11	0.19	0.14	0.14
query12	0.19	0.15	0.15
query13	0.46	0.47	0.47
query14	1.00	1.01	1.00
query15	0.61	0.59	0.59
query16	0.31	0.32	0.32
query17	1.11	1.10	1.09
query18	0.23	0.21	0.21
query19	2.06	1.98	1.93
query20	0.02	0.01	0.02
query21	15.44	0.23	0.14
query22	4.76	0.06	0.05
query23	16.13	0.30	0.12
query24	3.00	0.45	0.33
query25	0.10	0.05	0.04
query26	0.73	0.20	0.15
query27	0.05	0.03	0.04
query28	3.56	0.90	0.56
query29	12.47	4.34	3.48
query30	0.28	0.15	0.15
query31	2.77	0.59	0.31
query32	3.23	0.59	0.48
query33	3.16	3.27	3.22
query34	15.63	4.23	3.51
query35	3.55	3.53	3.52
query36	0.56	0.43	0.41
query37	0.10	0.07	0.07
query38	0.05	0.04	0.03
query39	0.04	0.03	0.03
query40	0.17	0.16	0.15
query41	0.08	0.03	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.04
Total cold run time: 96.68 s
Total hot run time: 25.27 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 47.20% (177/375) 🎉
Increment coverage report
Complete coverage report

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 82.13% (308/375) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 48.00% (180/375) 🎉
Increment coverage report
Complete coverage report

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

workbuddy + glm 5.2:

一、总体评价

这是一个高质量的外部元数据缓存重构。将旧的 MetaCache<T>(包装两个 LoadingCache)拆分为独立的 MetaCacheEntry<K,V>,配合不可变的 NameCacheValue 快照,架构清晰,并发控制严谨。未发现 blocking issue,可合入。

二、架构与优点

1. 缓存条目独立化
每个逻辑数据集(databaseNames / databases / tableNames / tables)成为独立 MetaCacheEntry,职责单一,TTL/容量/失效策略可独立配置。旧的 MetaCache<T>LoadingCache 耦合被消除。

2. 慢 I/O 不阻塞 Caffeine 同步 load 路径
getWithManualLoad 将慢 miss load 移出 Caffeine 的 CacheLoader.load,改用 striped loadLock 去重 + 手动 data.put。这使得 REFRESH CATALOG 不再被同 key 的慢 schema load 阻塞——回归测试 test_jdbc_refresh_catalog_schema_refresh_non_blocking 精确验证了这一点。

3. 三层并发协调机制设计正确
loadLock(去重)→ publishLock(序列化发布)→ generation(版本校验)三层配合:

  • 锁顺序全局一致(loadLock → publishLock 嵌套,无反向获取),无死锁风险
  • load 在 publishLock 之外执行,不阻塞 put/invalidate
  • generation 快照保证 invalidate 后 in-flight load 的 stale 结果不写回
  • invalidateAll/invalidateIfbumpAllGenerations 覆盖尚未入缓存的 in-flight load

4. replay 非阻塞语义
getDbForReplay / getTableForReplay 仅走 getIfPresent,miss 时不触发同步 load,避免 replayer 线程被远程 I/O 阻塞。

5. lower-case mode 0/1/2 行为正确
mode 0(原样)、mode 1(存储小写)、mode 2(比较不敏感)三条路径互斥,mode 2 统一走 resolveXxxNameFromSnapshot 做大小写索引查找。

6. NameCacheValue 不可变 + copy-on-write
names 列表与 lower-case 索引在同一快照中原子发布,读写无锁竞争。withName/withoutLocalName 生成新快照,读者始终看到一致状态。

7. 旧代码完全清理
MetaCache.javaLegacyMetaCacheFactory.java 删除后无遗留引用(已全局 grep 确认)。

三、问题与建议

[Minor] 第二次 generation 检查为 test-only 残留

MetaCacheEntry.java:332-334

putLoadedValueWithoutGenerationBump(key, loaded);
if (generation != generationOf(key)) {   // publishLock 持有期间不可能变
    removeLoadedValueWithoutGenerationBump(key, loaded);
}

此时已持有 publishLockgeneration 不可能被其他线程修改,第二次检查在生产环境恒为 false。该分支仅为配合 beforeManualCachePutForTest 测试钩子注入并发 invalidate 而保留。

建议:加注释说明此分支为 test-only 竞态模拟支持,避免被误判为防御式编程。或考虑将竞态验证收敛到测试子类内部,移除生产路径的冗余检查(符合 AGENTS.md "不做防御式编程"原则)。

[Nit] NameCacheValue.names() 每次深拷贝

names() 每次调用执行 copyPairs + ImmutableList.copyOf,分配新对象。ImmutableList 本身已不可变,若 Pair 调用方只读不写,可直接返回内部 ImmutableList 避免重复分配。属安全与性能的 trade-off,当前实现可接受。

[Nit] stats() 混合统计源

hitRate/evictionCount 来自 Caffeine(基于 getIfPresent 请求),loadSuccessCount/loadFailureCount 来自手动跟踪(manual miss load + auto refresh)。两者统计口径不同,展示时可能造成困惑。建议在 MetaCacheEntryStats 或注释中说明口径。

[Nit] Constructor 过载较多

MetaCacheEntry 有 6 个 public constructor + 2 个 static factory。可考虑 Builder 模式统一,但不影响正确性。

四、测试覆盖评估

测试覆盖非常充分,共 69 个单测 + 2 个回归测试:

测试文件 数量 覆盖重点
MetaCacheEntryTest 29 generation 竞态、striped lock 去重、invalidate 后不写回、refresh cancel、compute/put 并发发布
NameCacheValueTest 7 copy-on-write 不可变、case-insensitive 索引、冲突拒绝
MetaCacheDeadlockTest 1 嵌套 invalidateAll + bounded executor 死锁
ExternalCatalogTest 18 增量注册/注销、cold/hot miss、name miss refresh 可配置、replay 非阻塞
ExternalDatabaseTest 14 table 级对称覆盖、mode 0/1/2、replay hot snapshot miss
回归测试 2 JDBC refresh 非阻塞(debug point 注入慢 load)

核心竞态场景(invalidate vs in-flight load、put vs concurrent miss、refresh vs invalidate)均有对应单测。

五、核心结论:

  • Approve,架构重构质量高,并发控制设计正确,无死锁风险
  • 唯一值得跟进的是 MetaCacheEntry.getWithManualLoad 第二次 generation 检查的注释澄清(Minor,不影响正确性)
  • 测试覆盖全面(69 单测 + 2 回归测试),核心竞态场景均有验证

@wenzhenghu
wenzhenghu marked this pull request as ready for review July 8, 2026 01:45
@wenzhenghu

Copy link
Copy Markdown
Contributor Author

workbuddy + hy3:

核心结论:Approve(无 blocking issue,附 minor 跟进)

设计亮点(正面)

  • MetaCacheEntry 三层并发控制(loadLock 去重 → publishLock 序列化 → generation 版本校验)锁顺序一致、无死锁;慢 I/O 移出 Caffeine 同步 load 路径,REFRESH CATALOG 不被同 key 的慢加载阻塞。
  • getDbForReplay/getTableForReplay 严格仅 getIfPresent,replay 非阻塞。
  • 不可变 NameCacheValue + copy-on-write,快照读安全。
  • MetaCache/LegacyMetaCacheFactory 删除后全仓无遗留引用;lower-case mode 0/1/2 语义经统一名称快照后保持一致。

需关注/跟进的点

  1. replay 按 id 解析的健壮性(§4.1)getTableForReplay(long) 正确性依赖 registerTable 已填充 tableIdToName;而 ExternalDatabase.gsonPostProcessnewtableIdToName,未像 ExternalCatalog.gsonPostProcess 那样回填 dbIdToName,健壮性不一致。主路径无碍,建议补注释或在反序列化时回填。
  2. mode 2 + 自定义 meta-names 映射(§4.2):对象缓存以 local name 为 key、mode 2 查询以 remote name 为 key,remote≠local 时会走 load 路径。属历史遗留边角,本 PR 未引入新回归,建议补单测/注释明确已知限制。
  3. 次要(§4.3)buildMetaCache 双字段判重存在部分初始化竞态窗口;stats() 混合 Caffeine 与手动统计口径;构造函数重载过多。

测试覆盖充分MetaCacheEntryTest 通过覆写 beforeManualCachePutForTest/beforePublicMutationWriteForTest 钩子模拟"publish 窗口中触发 invalidateAll",验证了 stale-write-back 防护与 generation 协调;外加 ExternalCatalogTest/ExternalDatabaseTest/NameCacheValueTest 与 2 个 JDBC 回归用例。

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

workbuddy + kimi 2.7-code:

核心结论

  • 重构方向正确:MetaCacheEntry + NameCacheValue 拆分清晰,慢 I/O 移出 Caffeine 同步 load 路径,loadLock/publishLock/generation 三层并发协调严谨,replay 非阻塞。
  • 最新提交 142aa9f3afe 已修复运行时 id 映射维护问题,updateDatabaseCache/updateTableCache 会始终回填 dbIdToName/tableIdToName
  • 建议在合入前处理的问题ExternalCatalog.gsonPostProcess() 当前仅新建空 dbIdToName,未从已持久化的 dbNameToId 反填,FE 重启后首次按 id 的 replay/查询可能返回空。建议补充反填逻辑。

其余为 minor:mode 2 + meta-names 映射的 key 不一致(历史遗留)、buildMetaCache 双字段判重竞态、stats() 统计口径混合、构造函数重载过多、MetaCacheEntry 二次 generation 检查为 test-only 路径。

总体评价:Approve(建议合入,须处理 gsonPostProcess id 映射回填)

@morrySnow morrySnow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR refactors the external metadata cache from a monolithic MetaCache\<T\> wrapper into independent MetaCacheEntry instances with per-stripe generation-based concurrency control, a new NameCacheValue immutable snapshot, and centralized name-miss-refresh logic. The architecture is a substantial improvement in concurrency correctness. Below are the findings ranked by severity.

Finding 1: Mode-2 case-insensitive replay may fail after names-only invalidation

ExternalDatabase.java: getTableForReplay(String) (line ~735) and ExternalCatalog.java: getDbForReplay(String) (line ~266)

The old code stored the lower-case→original-case index in a separate ConcurrentHashMap that survived resetMetaCacheNames() (names-only invalidation). The new code embeds this index inside NameCacheValue, which is a Caffeine-cached snapshot. After resetMetaCacheNames() invalidates the names entry, getTableForReplay(String) fails to resolve case-insensitive names because resolveTableNameFromSnapshot(... isReplay=true) returns null without reloading. The exact-key check at line 717 (tables.getIfPresent(tblName)) provides partial protection but does NOT help when the replay caller passes a differently-cased name (e.g., "mytable" for cached key "MyTable"). Same issue exists at the database level.

Severity: Medium — requires concurrent names invalidation during replay in mode-2 catalogs.

Finding 2: NameCacheValue.remoteNameOfLocalName() is O(n) linear scan

NameCacheValue.java line 1838

Every local-to-remote name resolution scans all name pairs linearly. For catalogs with 10000+ databases/tables, this becomes expensive on hot paths like buildDbForInit() and buildTableForInit(). The constructor already builds a lowerCaseToRemoteName map for case-insensitive lookup; building a parallel localName→remoteName map would make this O(1).

Finding 3: NameCacheValue.containsLocalName() is O(n) linear scan

NameCacheValue.java line 1847

Similar to Finding 2 — a HashSet\<String\> of local names built during construction would give O(1) existence checks.

Finding 4: NameCacheValue.names() allocates N new Pair objects on every call

NameCacheValue.java line 1833

The names() method deep-copies all pairs via copyPairs() on every invocation. Called from listLocalDatabaseNamesFromCache() which runs on every getDbNames() / SHOW DATABASES / SHOW TABLES. The internal field is already an ImmutableList of already-copied Pairs — the second deep copy is wasteful for hot-path read operations.

Finding 5: invalidateAll() iterates per-key with per-key lock acquisition

MetaCacheEntry.java line 1606

The new invalidateAll() calls bumpAllGenerations() then iterates all keys calling invalidateKey() per key (each acquiring publishLock and bumping the stripe generation again — double bump). The old code used Caffeine's invalidateAll() which was O(1) from the caller's perspective. For caches with max_meta_object_cache_num=1000 entries, this means 1000 lock acquisitions with per-stripe contention.

Finding 6: Duplicated resolve/update/invalidation logic across ExternalCatalog and ExternalDatabase

ExternalCatalog.java lines 360-455, ExternalDatabase.java lines 829-916

Seven method pairs are structurally identical across the two classes: resolve*NameFromSnapshot(), get*NamesValue(), listLocal*NamesFromCache(), getRemote*Name(), update*Cache(), invalidate*Cache(), invalidateAll*Cache(). ~150 lines duplicated. Any future behavioral change to the miss-refresh policy or incremental update semantics must be applied in two places. Consider extracting a shared helper parameterized over the names/objects/ID fields.

Finding 7: Double generation bump in invalidateAll() and invalidateIf()

MetaCacheEntry.java lines 1612, 1639

invalidateAll() calls bumpAllGenerations() (which increments every stripe) and then invalidateKey() calls bumpGeneration(key) (which increments that stripe again). Each key's stripe generation is incremented twice — once by the bulk bump and once by the per-key bump. This overcounts invalidations and wastes atomic operations.


Overall assessment: The concurrency model improvements (per-stripe generations, publish locks, generation-aware refresh) are well-designed and address real correctness issues in the legacy code. The main concerns are: (1) mode-2 replay resilience after names invalidation, (2) O(n) scans in NameCacheValue on hot paths, and (3) significant code duplication between ExternalCatalog and ExternalDatabase. These are addressable without architectural changes.

@morrySnow morrySnow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Findings

This PR refactors the external metadata cache to use per-stripe generation-based concurrency control and MetaCacheEntry. The concurrency model is well-designed. Below are findings ranked by severity (inline comments on specific lines).

Correctness (3 findings)

  1. CONFIRMED: Lost load deduplication — No lock held through slow I/O; concurrent lookups for the same cold key trigger duplicate remote loads.
  2. CONFIRMED: Mode-2 case-insensitive replay regression — After names-only invalidation, replay lookups fail because the lower-case index moved into the Caffeine-cached snapshot.
  3. PLAUSIBLE: Race on getIfPresent-then-compute — Concurrent invalidateAll() can leave the names cache empty.

Efficiency (3 findings)

  1. O(n) linear scans in NameCacheValue (per lookup on hot paths)
  2. names() deep-copies all pairs on every call (hot path)
  3. invalidateAll() per-key lock acquisition + double generation bump

Code Quality (2 findings)

  1. unregisterTable may use wrong cache key in mode-2
  2. ~150 lines duplicated across ExternalCatalog and ExternalDatabase

@suxiaogang223 suxiaogang223 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Nit-level suggestions

Overall the architecture is sound and the concurrency model is well-designed. Approve. Below are four minor/nit suggestions for consideration:


1. [Nit] NameCacheValue.names() deep-copies all Pair objects on every call

NameCacheValue.javanames() method

public List<Pair<String, String>> names() {
    return ImmutableList.copyOf(copyPairs(names));
}

This is called from listLocalDatabaseNamesFromCache()getDbNames() on every SHOW DATABASES / SHOW TABLES. The internal field is already an ImmutableList of already-copied Pairs. The second copyPairs() creates N new Pair objects on every read. For catalogs with thousands of databases/tables, this allocates significant garbage on a hot path. Consider either:

  • Returning the internal ImmutableList directly with a documented contract that callers must not mutate Pair contents, or
  • Caching the defensive copy result.

2. [Nit] NameCacheValue.remoteNameOfLocalName() and containsLocalName() are O(n) linear scans

NameCacheValue.java lines 1841–1857

public String remoteNameOfLocalName(String localName) {
    for (Pair<String, String> pair : names) {
        if (pair.value().equals(localName)) {
            return pair.key();
        }
    }
    return null;
}

Every local-to-remote name resolution scans all pairs linearly. For catalogs with 10,000+ tables, this is expensive on hot paths like buildDbForInit() and buildTableForInit(). The constructor already builds a lowerCaseToRemoteName HashMap — building a parallel localName→remoteName HashMap (and a localName HashSet for containsLocalName) would make both methods O(1) at a small memory cost.

3. [Nit] Post-put generation re-check in getWithManualLoad is a test-only code path

MetaCacheEntry.javagetWithManualLoad() method

synchronized (publishLock(key)) {
    // ...
    putLoadedValueWithoutGenerationBump(key, loaded);
    if (generation != generationOf(key)) {   // ← unreachable in production
        removeLoadedValueWithoutGenerationBump(key, loaded);
    }
}

The generation cannot change while holding publishLock because the only code that bumps a stripe generation (bumpGeneration()) is called from methods that also hold publishLock for that key. This branch exists solely to support the beforeManualCachePutForTest test hook, which can trigger invalidateAll() from within the hook callback. Adding a comment here would prevent future readers from misinterpreting this as unnecessary defensive programming — something like:

// This re-check is reachable only in tests that inject an invalidateAll()
// via beforeManualCachePutForTest while publishLock is held.
if (generation != generationOf(key)) {
    removeLoadedValueWithoutGenerationBump(key, loaded);
}

4. [Nit] ~150 lines of duplicated logic across ExternalCatalog and ExternalDatabase

Seven method pairs are structurally identical between the two classes:

ExternalCatalog ExternalDatabase
resolveDatabaseNameFromSnapshot() resolveTableNameFromSnapshot()
getDatabaseNamesValue() getTableNamesValue()
listLocalDatabaseNamesFromCache() listLocalTableNamesFromCache()
getRemoteDatabaseName() getRemoteTableName()
updateDatabaseCache() updateTableCache()
invalidateDatabaseCache() invalidateTableCache()
invalidateAllDatabaseCache() invalidateAllTableCache()

Any future change to the miss-refresh policy or incremental update semantics must be applied in two places. Consider extracting a shared helper (e.g., ExternalMetaCacheCoordinator<K, V>) parameterized over the names entry, objects entry, and ID map fields.


Verdict: Approve. These are all minor code quality improvements — none affect correctness. Happy to see this land as-is, with these items as optional follow-up.

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@morrySnow

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR 65126 at head 157f67675ef23356b4881f1965a97f121b828d77.

I did not find a new non-duplicate blocking issue, so I am submitting this as a comment-only review with no inline comments.

Critical checkpoint conclusions:

  • Goal and scope: the patch consistently replaces the old MetaCache wrapper with MetaCacheEntry and immutable name snapshots across catalog, database, HMS, tests, and regression coverage.
  • Correctness: database/table name resolution, replay cache-only paths, and mode-2 case-insensitive fallbacks were checked against the changed code and existing review threads.
  • Concurrency: manual miss loading now holds the per-key load lock through slow load and publish, and invalidation/publish generation checks were reviewed in MetaCacheEntry.
  • Lifecycle: reset and removal-listener behavior remain explicit through cache invalidation/reset paths.
  • Configuration: the new name-miss-refresh and stripe-count configs were reviewed. The removed manual miss-load switch is ignored with a startup warning if stale in config files, while runtime mutation correctly rejects removed keys.
  • Compatibility: no persisted schema format change was found; replay paths avoid synchronous remote load-through.
  • Tests: added FE unit tests and regression cases cover the main cache/refactor paths, including replay and refresh behavior. I did not run FE tests locally because this checkout lacks thirdparty/installed and thirdparty/installed/bin/protoc.
  • Local validation: git diff --check 74e005f69050d993d1b480d13e55a4072554c4ba HEAD -- . ':(exclude).code-review.QoBYRt' passed.
  • Subagent convergence: both required subagents (optimizer-rewrite and tests-session-config) reported NO_NEW_VALUABLE_FINDINGS after the final empty comment set was recorded in the shared ledger.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 83.50% (334/400) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29387 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 157f67675ef23356b4881f1965a97f121b828d77, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17626	4043	4050	4043
q2	2076	314	198	198
q3	10289	1418	820	820
q4	4701	475	339	339
q5	7571	965	603	603
q6	183	173	137	137
q7	767	806	629	629
q8	9347	1548	1502	1502
q9	5583	4394	4334	4334
q10	6732	1790	1537	1537
q11	499	342	311	311
q12	732	552	429	429
q13	18149	3901	2759	2759
q14	270	257	238	238
q15	q16	787	786	713	713
q17	1041	915	1050	915
q18	6795	5808	5576	5576
q19	1311	1266	1039	1039
q20	712	649	519	519
q21	6039	2677	2431	2431
q22	439	360	315	315
Total cold run time: 101649 ms
Total hot run time: 29387 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4328	4256	4274	4256
q2	281	319	214	214
q3	4604	4950	4424	4424
q4	2056	2179	1395	1395
q5	4417	4285	4293	4285
q6	225	177	126	126
q7	1725	1923	1895	1895
q8	2620	2140	2148	2140
q9	8087	8302	7843	7843
q10	4775	4723	4271	4271
q11	593	422	399	399
q12	754	771	537	537
q13	3203	3724	2926	2926
q14	299	292	271	271
q15	q16	720	746	667	667
q17	1357	1322	1313	1313
q18	8178	7607	7274	7274
q19	1148	1088	1093	1088
q20	2202	2212	1934	1934
q21	5258	4607	4468	4468
q22	517	460	397	397
Total cold run time: 57347 ms
Total hot run time: 52123 ms

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29518 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 2046bea24a97e2c6b134dcdada422c5bdab00fdd, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17618	4050	4098	4050
q2	2048	310	203	203
q3	10252	1452	826	826
q4	4672	479	341	341
q5	7563	832	558	558
q6	187	175	138	138
q7	740	826	598	598
q8	9315	1547	1684	1547
q9	5849	4300	4357	4300
q10	6764	1736	1478	1478
q11	514	365	320	320
q12	715	582	452	452
q13	18110	3807	2715	2715
q14	268	266	248	248
q15	q16	795	775	702	702
q17	1027	1056	1012	1012
q18	6947	5804	5456	5456
q19	1305	1331	1064	1064
q20	827	680	572	572
q21	6289	2931	2620	2620
q22	458	409	318	318
Total cold run time: 102263 ms
Total hot run time: 29518 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	5286	4886	4985	4886
q2	290	325	202	202
q3	5014	5298	4705	4705
q4	2120	2153	1367	1367
q5	4785	4842	4592	4592
q6	238	180	135	135
q7	2024	1797	1538	1538
q8	2422	2175	2219	2175
q9	7648	7261	7176	7176
q10	4636	4590	4112	4112
q11	533	385	350	350
q12	738	743	529	529
q13	3031	3335	2775	2775
q14	280	284	261	261
q15	q16	669	699	606	606
q17	1294	1269	1252	1252
q18	7487	7040	6956	6956
q19	1097	1070	1101	1070
q20	2221	2205	1977	1977
q21	5229	4660	4481	4481
q22	518	471	391	391
Total cold run time: 57560 ms
Total hot run time: 51536 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 178256 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 2046bea24a97e2c6b134dcdada422c5bdab00fdd, data reload: false

query5	4311	619	481	481
query6	454	221	205	205
query7	4839	596	338	338
query8	333	196	185	185
query9	8771	4015	3998	3998
query10	459	353	305	305
query11	5866	2339	2131	2131
query12	161	105	100	100
query13	1243	641	435	435
query14	6255	5202	4916	4916
query14_1	4269	4240	4212	4212
query15	212	200	175	175
query16	990	460	361	361
query17	921	689	559	559
query18	2425	464	334	334
query19	202	183	149	149
query20	108	106	104	104
query21	230	159	134	134
query22	13596	13658	13337	13337
query23	17396	16611	16146	16146
query23_1	16261	16324	16133	16133
query24	7519	1783	1271	1271
query24_1	1296	1289	1293	1289
query25	552	425	374	374
query26	1363	348	209	209
query27	2610	557	385	385
query28	4492	1969	2012	1969
query29	1048	617	498	498
query30	349	271	221	221
query31	1142	1088	1009	1009
query32	122	74	63	63
query33	532	335	256	256
query34	1194	1123	632	632
query35	766	792	677	677
query36	1152	1191	1052	1052
query37	160	113	100	100
query38	1890	1713	1666	1666
query39	907	871	850	850
query39_1	847	835	850	835
query40	255	167	148	148
query41	73	68	71	68
query42	95	94	102	94
query43	321	328	282	282
query44	1444	782	778	778
query45	196	187	178	178
query46	1050	1245	734	734
query47	2079	2113	1975	1975
query48	422	396	308	308
query49	584	427	322	322
query50	1077	417	329	329
query51	10931	10873	10970	10873
query52	86	93	78	78
query53	261	294	213	213
query54	294	253	242	242
query55	76	74	69	69
query56	323	323	307	307
query57	1311	1266	1195	1195
query58	316	263	279	263
query59	1558	1649	1485	1485
query60	308	270	246	246
query61	149	148	149	148
query62	546	495	434	434
query63	235	203	208	203
query64	2838	1038	849	849
query65	4682	4679	4646	4646
query66	1852	496	392	392
query67	29323	29251	29357	29251
query68	3099	1521	985	985
query69	406	300	260	260
query70	1088	976	958	958
query71	401	353	310	310
query72	3201	2696	2431	2431
query73	865	780	431	431
query74	5114	4970	4751	4751
query75	2515	2486	2137	2137
query76	2343	1183	770	770
query77	343	372	282	282
query78	11820	11707	11354	11354
query79	1377	1169	754	754
query80	664	553	458	458
query81	480	363	296	296
query82	569	157	124	124
query83	406	330	302	302
query84	325	159	128	128
query85	936	637	536	536
query86	361	297	276	276
query87	1827	1819	1757	1757
query88	3726	2794	2769	2769
query89	447	379	343	343
query90	1960	200	197	197
query91	202	187	163	163
query92	67	62	54	54
query93	1608	1578	959	959
query94	544	370	322	322
query95	816	584	462	462
query96	1127	785	361	361
query97	2642	2625	2474	2474
query98	213	210	205	205
query99	1089	1107	997	997
Total cold run time: 262879 ms
Total hot run time: 178256 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 25.05 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 2046bea24a97e2c6b134dcdada422c5bdab00fdd, data reload: false

query1	0.01	0.01	0.01
query2	0.10	0.05	0.05
query3	0.29	0.13	0.14
query4	1.62	0.15	0.14
query5	0.25	0.22	0.23
query6	1.24	1.08	1.08
query7	0.04	0.01	0.00
query8	0.06	0.04	0.04
query9	0.39	0.32	0.32
query10	0.56	0.56	0.53
query11	0.18	0.14	0.14
query12	0.19	0.15	0.14
query13	0.47	0.48	0.48
query14	1.03	1.00	1.02
query15	0.63	0.58	0.60
query16	0.32	0.34	0.32
query17	1.10	1.08	1.09
query18	0.23	0.21	0.22
query19	2.12	1.94	1.93
query20	0.01	0.02	0.02
query21	15.44	0.19	0.14
query22	4.99	0.06	0.05
query23	16.12	0.32	0.12
query24	2.93	0.43	0.33
query25	0.11	0.05	0.04
query26	0.73	0.20	0.16
query27	0.04	0.04	0.03
query28	3.60	0.92	0.53
query29	12.52	4.12	3.28
query30	0.27	0.17	0.16
query31	2.77	0.58	0.31
query32	3.24	0.60	0.48
query33	3.32	3.23	3.20
query34	15.71	4.25	3.50
query35	3.48	3.52	3.52
query36	0.55	0.45	0.43
query37	0.09	0.07	0.06
query38	0.04	0.04	0.04
query39	0.03	0.02	0.02
query40	0.18	0.15	0.15
query41	0.10	0.03	0.02
query42	0.04	0.03	0.03
query43	0.04	0.03	0.04
Total cold run time: 97.18 s
Total hot run time: 25.05 s

@924060929

Copy link
Copy Markdown
Contributor

/review

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 25th, 2026 6:03 AM.
Workflow run: https://github.com/apache/doris/actions/runs/29812155319

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29146 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 7533c1d16eb7158b9d0a638198ddb20d7fddc725, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17621	4314	4030	4030
q2	2042	323	201	201
q3	10229	1427	822	822
q4	4673	453	337	337
q5	7566	827	566	566
q6	186	171	143	143
q7	744	806	611	611
q8	9784	1518	1472	1472
q9	6168	4310	4251	4251
q10	6829	1724	1445	1445
q11	509	349	330	330
q12	756	577	459	459
q13	18117	3296	2781	2781
q14	275	258	247	247
q15	q16	781	778	717	717
q17	1001	940	1044	940
q18	6855	5903	5450	5450
q19	1430	1231	1090	1090
q20	804	677	595	595
q21	5903	2620	2358	2358
q22	433	351	301	301
Total cold run time: 102706 ms
Total hot run time: 29146 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4376	4294	4299	4294
q2	281	324	220	220
q3	4528	4944	4373	4373
q4	2015	2136	1340	1340
q5	4408	4259	4242	4242
q6	221	175	131	131
q7	1741	1958	1717	1717
q8	2587	2230	2135	2135
q9	7891	7773	7773	7773
q10	4660	4620	4205	4205
q11	555	438	386	386
q12	738	855	627	627
q13	3266	3596	3019	3019
q14	325	313	268	268
q15	q16	713	763	645	645
q17	1349	1337	1314	1314
q18	7932	7395	7015	7015
q19	1128	1064	1062	1062
q20	2181	2240	1953	1953
q21	5182	4613	4436	4436
q22	516	466	442	442
Total cold run time: 56593 ms
Total hot run time: 51597 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 178004 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 7533c1d16eb7158b9d0a638198ddb20d7fddc725, data reload: false

query5	4313	623	479	479
query6	463	225	208	208
query7	4882	571	325	325
query8	339	188	175	175
query9	8751	4018	3991	3991
query10	458	359	301	301
query11	5884	2342	2117	2117
query12	158	102	97	97
query13	1265	573	429	429
query14	6241	5192	4853	4853
query14_1	4249	4205	4192	4192
query15	211	210	177	177
query16	1069	500	469	469
query17	1129	707	587	587
query18	2515	471	350	350
query19	220	190	156	156
query20	114	108	108	108
query21	235	164	139	139
query22	13615	13534	13471	13471
query23	17403	16713	16324	16324
query23_1	16266	16213	16216	16213
query24	7712	1770	1302	1302
query24_1	1351	1291	1256	1256
query25	579	468	406	406
query26	1369	367	221	221
query27	2544	566	373	373
query28	4403	1947	1968	1947
query29	1080	631	495	495
query30	345	268	232	232
query31	1119	1092	974	974
query32	114	62	62	62
query33	534	318	268	268
query34	1175	1091	647	647
query35	758	785	668	668
query36	1220	1192	1065	1065
query37	157	121	95	95
query38	1885	1709	1668	1668
query39	881	866	853	853
query39_1	828	846	839	839
query40	253	207	139	139
query41	65	65	65	65
query42	93	95	90	90
query43	319	321	274	274
query44	1387	766	745	745
query45	191	184	188	184
query46	1018	1172	727	727
query47	2166	2103	2028	2028
query48	370	412	278	278
query49	577	416	300	300
query50	1073	445	369	369
query51	10876	10979	10707	10707
query52	85	84	72	72
query53	266	284	201	201
query54	286	242	219	219
query55	73	73	69	69
query56	293	301	285	285
query57	1318	1295	1232	1232
query58	263	240	255	240
query59	1585	1619	1434	1434
query60	299	279	260	260
query61	161	157	152	152
query62	550	501	421	421
query63	246	202	200	200
query64	2844	1055	916	916
query65	4688	4628	4647	4628
query66	1835	508	375	375
query67	29191	29215	28978	28978
query68	3187	1516	931	931
query69	402	303	265	265
query70	1010	935	943	935
query71	368	324	317	317
query72	3086	2367	2370	2367
query73	812	790	438	438
query74	5049	4870	4713	4713
query75	2499	2572	2125	2125
query76	2350	1125	783	783
query77	344	370	280	280
query78	11789	11726	11433	11433
query79	1385	1203	726	726
query80	1305	557	470	470
query81	522	330	288	288
query82	598	155	118	118
query83	367	322	296	296
query84	275	161	129	129
query85	953	598	544	544
query86	430	300	283	283
query87	1841	1825	1758	1758
query88	3671	2757	2730	2730
query89	443	373	336	336
query90	1900	201	195	195
query91	203	186	161	161
query92	64	59	54	54
query93	1653	1562	958	958
query94	718	349	330	330
query95	779	511	568	511
query96	1071	751	312	312
query97	2627	2627	2470	2470
query98	210	203	200	200
query99	1113	1114	974	974
Total cold run time: 263319 ms
Total hot run time: 178004 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.9 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 7533c1d16eb7158b9d0a638198ddb20d7fddc725, data reload: false

query1	0.01	0.01	0.01
query2	0.10	0.04	0.04
query3	0.25	0.14	0.13
query4	1.60	0.14	0.14
query5	0.23	0.22	0.21
query6	1.27	1.03	1.10
query7	0.04	0.00	0.00
query8	0.05	0.04	0.04
query9	0.36	0.31	0.31
query10	0.54	0.58	0.54
query11	0.19	0.14	0.14
query12	0.17	0.14	0.14
query13	0.46	0.48	0.50
query14	1.03	1.02	1.01
query15	0.61	0.59	0.60
query16	0.32	0.32	0.32
query17	1.10	1.15	1.12
query18	0.23	0.21	0.21
query19	2.00	1.89	1.98
query20	0.02	0.01	0.02
query21	15.43	0.21	0.13
query22	4.85	0.06	0.05
query23	16.11	0.32	0.12
query24	2.91	0.41	0.30
query25	0.13	0.05	0.04
query26	0.72	0.21	0.16
query27	0.05	0.04	0.03
query28	3.54	0.92	0.53
query29	12.49	4.15	3.32
query30	0.27	0.15	0.16
query31	2.77	0.60	0.32
query32	3.22	0.59	0.48
query33	3.23	3.14	3.20
query34	15.60	4.30	3.49
query35	3.60	3.51	3.53
query36	0.55	0.44	0.42
query37	0.10	0.06	0.07
query38	0.05	0.03	0.04
query39	0.04	0.03	0.03
query40	0.18	0.16	0.14
query41	0.08	0.04	0.03
query42	0.04	0.03	0.03
query43	0.04	0.03	0.04
Total cold run time: 96.58 s
Total hot run time: 24.9 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 86.59% (452/522) 🎉
Increment coverage report
Complete coverage report

@yiguolei

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review completed for PR #65126 at head 7533c1d16eb7158b9d0a638198ddb20d7fddc725.

I found two patch-scoped P2 performance regressions that should be addressed:

  • Delegated Iceberg REST isTableExist() now enumerates the whole database before its point existence request in the ordinary no-mapping mode.
  • Selective cache invalidation advances every generation stripe, preventing unrelated in-flight metadata loads from being cached.

Critical checkpoint conclusions:

  • Goal and test intent: The refactor cleanly separates external names, objects, retained IDs, and engine-cache entries, and adds substantial concurrency, replay, event, and session-bypass coverage. The two reported cases are not covered by the added tests.
  • Focus and scope: No additional user focus was provided. I reviewed all 30 changed files plus the relevant catalog, replay, Iceberg REST, engine-cache, Caffeine, configuration, and constraint call chains.
  • Concurrency and locks: Manual miss publication, refresh publication, public mutations, bulk invalidation, synchronous removal callbacks, and event-lock boundaries were traced. Besides the selective-invalidation issue reported inline, no new non-duplicate deadlock or stale-publication regression survived validation; the capacity-eviction/refresh race is already covered by existing thread r3599912105.
  • Lifecycle and cleanup: Names/object/ID/engine hot-cold combinations, TTL/capacity eviction, reset, CREATE/DROP/rename, and cache-only replay were checked. Current patch-scoped cleanup paths are consistent after the fixes already discussed on this PR.
  • Configuration and compatibility: Mutable name-miss behavior, removed legacy cache configuration, immutable list exposure, lower-case modes 0/1/2, name mapping, and current/legacy refresh edit logs were reviewed. No additional patch-scoped compatibility issue was established.
  • Parallel paths and conditionals: HMS events, manual refresh, leader logging, follower replay, delegated Iceberg REST access, ordinary shared-cache access, canonical local names, deterministic IDs, and engine-specific invalidation were compared.
  • Tests and validation: Review was static as required by the review instructions; no local build or test was run. Current CI reports compile, FE unit tests, external regression, P0 regression, and checkstyle passing. The FE coverage status is failing despite a posted 86.59% incremental-line report, and the automated code-review check is still pending.
  • Observability, persistence, and failover: Cache statistics, invalidation logging, removal callbacks, edit-log replay, and retained-ID recovery were inspected; no additional patch-scoped issue was found.
  • Data writes and variables: This change does not introduce a durable data-write path. Session context, remote/local database and table names, IDs, generation values, and update timestamps were traced through their consumers.
  • Performance and other issues: The two new regressions are inline. The separate O(n) copy-on-write HMS name-update cost is already covered by r3602162049 and issue #65779. Duplicate, fixed, pre-existing, and unproven candidates were suppressed.

Three independent convergence lanes re-reviewed the current two-comment set and returned NO_NEW_VALUABLE_FINDINGS.

Comment thread fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java Outdated
### What problem does this PR solve?

Issue Number: None

Related PR: apache#65126

Problem Summary: Delegated Iceberg REST session table-existence checks enumerated all tables even when the requested name could be used directly. Keep mode 0 as a direct point lookup, and enumerate without shared caches only when lower-case or explicit name mapping requires local-to-remote resolution.

### Release note

Reduce unnecessary Iceberg REST metadata enumeration for delegated session table-existence checks.

### Check List (For Author)

- Test: Unit Test
    - ExternalDatabaseSessionContextTest: 8 tests passed
    - ExternalDatabaseTest: 22 tests passed
    - FE checkstyle passed
- Behavior changed: Yes. Mode 0 delegated session existence checks now use a point lookup.
- Does this need documentation: No
@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29658 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f2e6cfc7c116525f115e95005ba3eeeeabc935a2, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17600	4237	4090	4090
q2	2018	325	203	203
q3	10301	1443	832	832
q4	4675	473	344	344
q5	7508	859	572	572
q6	197	177	141	141
q7	758	822	614	614
q8	9344	1520	1654	1520
q9	5573	4364	4399	4364
q10	6760	1717	1497	1497
q11	520	353	331	331
q12	724	575	474	474
q13	18087	3445	2717	2717
q14	270	264	244	244
q15	q16	787	779	705	705
q17	996	922	971	922
q18	6874	5856	5656	5656
q19	1331	1294	1125	1125
q20	792	687	602	602
q21	5910	2669	2404	2404
q22	423	355	301	301
Total cold run time: 101448 ms
Total hot run time: 29658 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4568	4493	4473	4473
q2	306	323	212	212
q3	4550	4992	4414	4414
q4	2058	2204	1354	1354
q5	4378	4274	4285	4274
q6	236	178	133	133
q7	1741	2255	1799	1799
q8	2679	2332	2306	2306
q9	8078	8095	7734	7734
q10	4673	4649	4233	4233
q11	577	437	407	407
q12	752	796	558	558
q13	3249	3629	2984	2984
q14	296	315	275	275
q15	q16	698	767	654	654
q17	1383	1362	1356	1356
q18	8121	7438	7334	7334
q19	1179	1152	1131	1131
q20	2225	2208	1934	1934
q21	5267	4648	4524	4524
q22	521	477	417	417
Total cold run time: 57535 ms
Total hot run time: 52506 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 178689 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f2e6cfc7c116525f115e95005ba3eeeeabc935a2, data reload: false

query5	4307	641	485	485
query6	463	223	215	215
query7	4841	587	351	351
query8	337	191	180	180
query9	8760	4085	4071	4071
query10	476	380	304	304
query11	5759	2356	2127	2127
query12	160	106	100	100
query13	1260	639	428	428
query14	6215	5330	5047	5047
query14_1	4354	4319	4333	4319
query15	208	203	186	186
query16	1032	507	417	417
query17	947	732	591	591
query18	2435	499	354	354
query19	214	191	151	151
query20	114	108	104	104
query21	234	160	137	137
query22	13581	13644	13390	13390
query23	17365	16524	16161	16161
query23_1	16303	16289	16179	16179
query24	7510	1765	1291	1291
query24_1	1332	1260	1310	1260
query25	577	470	420	420
query26	1354	346	226	226
query27	2608	571	391	391
query28	4510	1992	1974	1974
query29	1079	630	503	503
query30	341	260	236	236
query31	1112	1090	975	975
query32	115	67	62	62
query33	530	337	264	264
query34	1150	1144	642	642
query35	787	792	687	687
query36	1207	1177	1034	1034
query37	155	113	99	99
query38	1886	1700	1649	1649
query39	883	868	860	860
query39_1	843	838	831	831
query40	287	161	149	149
query41	64	63	63	63
query42	94	90	92	90
query43	340	342	297	297
query44	1426	761	762	761
query45	195	179	176	176
query46	1077	1196	737	737
query47	2101	2122	1960	1960
query48	409	438	295	295
query49	588	416	312	312
query50	1115	426	343	343
query51	10698	10593	10618	10593
query52	85	89	75	75
query53	260	274	213	213
query54	301	254	217	217
query55	76	73	67	67
query56	311	319	309	309
query57	1317	1288	1185	1185
query58	287	278	279	278
query59	1645	1681	1445	1445
query60	307	279	241	241
query61	155	150	179	150
query62	536	495	432	432
query63	247	204	205	204
query64	2837	1022	873	873
query65	4672	4623	4629	4623
query66	1867	500	416	416
query67	29339	29306	29205	29205
query68	3087	1620	1032	1032
query69	410	311	274	274
query70	1072	999	970	970
query71	353	342	317	317
query72	3013	2701	2330	2330
query73	861	840	453	453
query74	5050	4913	4717	4717
query75	2529	2504	2142	2142
query76	2331	1186	788	788
query77	359	386	287	287
query78	11975	11793	11333	11333
query79	1411	1199	756	756
query80	1264	564	479	479
query81	499	331	291	291
query82	551	163	127	127
query83	401	325	301	301
query84	325	163	134	134
query85	991	593	543	543
query86	402	302	264	264
query87	1821	1834	1774	1774
query88	3729	2834	2799	2799
query89	442	381	323	323
query90	1858	221	207	207
query91	203	204	162	162
query92	65	62	56	56
query93	1632	1457	913	913
query94	663	366	332	332
query95	800	524	485	485
query96	1067	817	363	363
query97	2614	2636	2530	2530
query98	218	209	203	203
query99	1090	1117	946	946
Total cold run time: 263416 ms
Total hot run time: 178689 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 25 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit f2e6cfc7c116525f115e95005ba3eeeeabc935a2, data reload: false

query1	0.00	0.00	0.00
query2	0.10	0.04	0.05
query3	0.25	0.14	0.13
query4	1.60	0.13	0.14
query5	0.24	0.23	0.23
query6	1.25	1.06	1.02
query7	0.04	0.01	0.01
query8	0.06	0.04	0.04
query9	0.37	0.31	0.32
query10	0.56	0.54	0.55
query11	0.19	0.13	0.14
query12	0.19	0.14	0.15
query13	0.47	0.48	0.47
query14	1.01	1.02	1.00
query15	0.61	0.59	0.58
query16	0.31	0.33	0.33
query17	1.14	1.13	1.09
query18	0.23	0.21	0.21
query19	2.06	2.00	1.94
query20	0.02	0.01	0.01
query21	15.43	0.23	0.13
query22	4.67	0.05	0.06
query23	16.15	0.30	0.12
query24	2.95	0.43	0.33
query25	0.23	0.05	0.04
query26	0.71	0.20	0.15
query27	0.05	0.04	0.03
query28	3.58	0.92	0.54
query29	12.50	4.16	3.28
query30	0.27	0.16	0.15
query31	2.77	0.62	0.32
query32	3.26	0.58	0.49
query33	3.18	3.26	3.21
query34	15.59	4.22	3.54
query35	3.52	3.48	3.52
query36	0.55	0.45	0.44
query37	0.09	0.07	0.06
query38	0.05	0.04	0.03
query39	0.04	0.03	0.02
query40	0.18	0.16	0.15
query41	0.08	0.04	0.03
query42	0.04	0.03	0.03
query43	0.05	0.03	0.03
Total cold run time: 96.64 s
Total hot run time: 25 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 87.29% (460/527) 🎉
Increment coverage report
Complete coverage report

@wenzhenghu

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29030 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17754	4014	3980	3980
q2	2002	312	213	213
q3	10292	1403	832	832
q4	4707	463	334	334
q5	7606	849	547	547
q6	199	167	139	139
q7	748	797	616	616
q8	10121	1533	1579	1533
q9	5945	4332	4322	4322
q10	6840	1726	1488	1488
q11	508	362	321	321
q12	763	570	450	450
q13	18120	3262	2710	2710
q14	258	253	232	232
q15	q16	781	764	702	702
q17	1029	967	1055	967
q18	6889	5816	5435	5435
q19	1274	1199	1035	1035
q20	864	665	540	540
q21	5619	2643	2337	2337
q22	431	351	297	297
Total cold run time: 102750 ms
Total hot run time: 29030 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4426	4329	4353	4329
q2	275	311	207	207
q3	4540	4948	4414	4414
q4	2019	2100	1384	1384
q5	4362	4224	4225	4224
q6	224	171	126	126
q7	1727	1854	1767	1767
q8	2478	2164	2055	2055
q9	7716	7729	7634	7634
q10	4659	4615	4192	4192
q11	569	400	375	375
q12	743	749	524	524
q13	3416	3676	2967	2967
q14	304	306	277	277
q15	q16	741	720	627	627
q17	1338	1320	1303	1303
q18	7970	7258	6923	6923
q19	1082	1057	1061	1057
q20	2179	2202	1925	1925
q21	5162	4538	4369	4369
q22	497	454	395	395
Total cold run time: 56427 ms
Total hot run time: 51074 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 177027 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802, data reload: false

query5	4328	615	483	483
query6	469	216	203	203
query7	4891	593	341	341
query8	332	186	165	165
query9	8757	4058	4020	4020
query10	521	366	287	287
query11	5902	2313	2120	2120
query12	157	97	97	97
query13	1254	602	408	408
query14	6217	5174	4837	4837
query14_1	4200	4214	4213	4213
query15	225	210	177	177
query16	1047	468	428	428
query17	1100	694	543	543
query18	2483	481	335	335
query19	207	188	153	153
query20	117	107	105	105
query21	229	159	136	136
query22	13570	13545	13287	13287
query23	17240	16449	16134	16134
query23_1	16301	16252	16237	16237
query24	7392	1773	1273	1273
query24_1	1281	1300	1276	1276
query25	563	474	388	388
query26	1350	351	203	203
query27	2571	588	381	381
query28	4481	2009	1996	1996
query29	1164	605	461	461
query30	340	265	236	236
query31	1106	1080	978	978
query32	105	60	66	60
query33	528	314	247	247
query34	1188	1137	658	658
query35	762	771	660	660
query36	1179	1174	1052	1052
query37	147	103	91	91
query38	1871	1691	1640	1640
query39	897	857	823	823
query39_1	831	827	839	827
query40	244	167	136	136
query41	63	61	62	61
query42	93	88	89	88
query43	335	327	276	276
query44	1468	760	752	752
query45	191	177	177	177
query46	1052	1161	699	699
query47	2161	2109	2029	2029
query48	393	368	279	279
query49	559	418	316	316
query50	1120	436	324	324
query51	10806	10779	10605	10605
query52	89	85	75	75
query53	263	288	198	198
query54	284	239	215	215
query55	76	70	65	65
query56	304	281	288	281
query57	1328	1285	1210	1210
query58	301	261	244	244
query59	1584	1605	1411	1411
query60	311	265	249	249
query61	152	140	148	140
query62	563	506	421	421
query63	242	201	198	198
query64	2894	1135	977	977
query65	4724	4618	4580	4580
query66	1861	524	385	385
query67	29412	29094	28994	28994
query68	3485	1513	1040	1040
query69	415	316	291	291
query70	1043	930	925	925
query71	376	331	313	313
query72	3223	2684	2308	2308
query73	799	777	426	426
query74	5068	4878	4742	4742
query75	2542	2497	2099	2099
query76	2313	1149	756	756
query77	347	369	287	287
query78	11861	11926	11248	11248
query79	1481	1145	760	760
query80	1300	577	468	468
query81	549	329	296	296
query82	595	153	118	118
query83	373	333	297	297
query84	277	156	130	130
query85	974	603	535	535
query86	421	299	254	254
query87	1825	1817	1758	1758
query88	3733	2776	2769	2769
query89	434	371	332	332
query90	1948	189	198	189
query91	200	200	158	158
query92	61	62	51	51
query93	1684	1667	945	945
query94	732	356	308	308
query95	780	608	456	456
query96	1092	784	330	330
query97	2641	2649	2481	2481
query98	211	219	196	196
query99	1096	1111	970	970
Total cold run time: 264435 ms
Total hot run time: 177027 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 25.13 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802, data reload: false

query1	0.01	0.01	0.00
query2	0.09	0.04	0.05
query3	0.26	0.13	0.13
query4	1.61	0.14	0.14
query5	0.24	0.22	0.22
query6	1.25	1.12	1.12
query7	0.03	0.01	0.00
query8	0.06	0.04	0.04
query9	0.38	0.31	0.31
query10	0.54	0.59	0.59
query11	0.19	0.13	0.13
query12	0.18	0.15	0.14
query13	0.47	0.47	0.50
query14	1.01	1.00	1.01
query15	0.61	0.62	0.58
query16	0.32	0.31	0.33
query17	1.14	1.11	1.11
query18	0.22	0.20	0.21
query19	2.06	2.00	1.98
query20	0.01	0.02	0.01
query21	15.45	0.19	0.13
query22	5.04	0.05	0.06
query23	16.13	0.31	0.12
query24	3.00	0.41	0.32
query25	0.10	0.06	0.04
query26	0.75	0.20	0.16
query27	0.03	0.04	0.03
query28	3.48	0.92	0.52
query29	12.53	4.05	3.29
query30	0.28	0.15	0.16
query31	2.77	0.59	0.32
query32	3.22	0.58	0.50
query33	3.23	3.18	3.18
query34	15.64	4.18	3.54
query35	3.54	3.50	3.50
query36	0.56	0.43	0.42
query37	0.09	0.07	0.06
query38	0.04	0.04	0.03
query39	0.04	0.04	0.03
query40	0.18	0.15	0.15
query41	0.09	0.03	0.02
query42	0.03	0.02	0.02
query43	0.04	0.04	0.03
Total cold run time: 96.94 s
Total hot run time: 25.13 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 87.29% (460/527) 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants