branch-4.1: [fix](fe) Prevent stale external name publication after refresh - #67417
branch-4.1: [fix](fe) Prevent stale external name publication after refresh#67417924060929 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes because the custom name-loading protocol still has correctness gaps in the production cache lifecycle.
- Concurrency and refresh: the new generation fence covers the manual cold-load publication only. Caffeine's configured
refreshAfterWritepath can still reinsert after invalidate/reset, and incremental update/removal is not atomic with cold loading in either ordering direction. - Caller state: production loaders mutate case-folding side maps before the fence, and a rejected load can return a list without satisfying the cache-only
getRemoteNamecontract used by mapped/lowercased database and table construction. - Compatibility and tests: direct loader adaptation loses Caffeine's interrupt restoration. The new test is useful for the cold-miss/invalidate-all race, but disables production refresh, omits incremental and mapped-name paths, and does not protect executor cleanup around its first await.
- Deadlock/performance checkpoint: remote connector I/O remains outside
namesMutationLock, and the synchronous meta-object removal-listener protection is unchanged; I found no new bounded-executor/listener deadlock or separate lock-order cycle. - Scope and hygiene: both changed files and their catalog/database callers were reviewed, existing review context contains no prior inline findings, and
git diff --checkis clean. No additional user focus was provided. Builds/tests were not run because the review runner explicitly forbids builds.
Review status: complete after two review rounds and convergence on the six inline findings above.
|
|
||
| public List<String> listNames() { | ||
| return Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList()); | ||
| List<Pair<String, String>> names = namesCache.getIfPresent(""); |
There was a problem hiding this comment.
[P1] Fence automatic refresh publication too
In Caffeine 2.9.3 this getIfPresent("") is not passive: an eligible hit runs afterRead -> refreshIfNeeded. If that async reload starts before invalidateAll()/resetNames(), its completion sees the mapping as absent and inserts the loaded value without consulting namesGeneration or namesMutationLock. Both production callers enable refreshAfterWrite, while the new test disables it, so REFRESH CATALOG can still be undone by a pre-refresh reload. Please route automatic refresh through the same generation-aware publication path (or disable Caffeine-owned refresh and implement a fenced reload), and cover the production refresh configuration in the test.
| synchronized (namesLoadLock) { | ||
| names = namesCache.getIfPresent(""); | ||
| if (names == null) { | ||
| long loadGeneration = namesGeneration.get(); |
There was a problem hiding this comment.
[P1] Make cold loads atomic with incremental mutations
This generation sample is not ordered with the miss check or the incremental mutation. If updateCache/invalidate runs after the sample, it installs a singleton/empty entry, the full load is rejected, and later reads hit that incomplete entry instead of retrying. In the reverse window, a mutation can increment/publish between line 96 and this sample; the loader then observes the new generation and later overwrites that explicit mutation. The old same-key Caffeine load/compute ordering serialized both cases. Please make the sample plus cache recheck atomic with namesMutationLock and ensure an incremental mutation cannot turn an incomplete cold load into an authoritative singleton/empty entry; add paused-load tests for both update and per-name invalidate.
| names = namesCache.getIfPresent(""); | ||
| if (names == null) { | ||
| long loadGeneration = namesGeneration.get(); | ||
| names = loadNames(); |
There was a problem hiding this comment.
[P1] Fence the loaders' side effects as well
Rejecting the returned list here does not reject everything the load published. The catalog loader clears/fills lowerCaseToDatabaseName, and the table loader clears/fills lowerCaseToTableName, before this generation check. A pre-refresh load can therefore resume after the reset, repopulate the new/cleared case-routing map with its old snapshot, and still be rejected only from namesCache; case-insensitive lookups then trust that stale hit without reloading. Please make the loaders side-effect free and publish the derived map with the list under the same generation fence, or otherwise discard/rebuild those maps when the load loses the generation check.
| } | ||
| } | ||
| } | ||
| return names.stream().map(Pair::value).collect(Collectors.toList()); |
There was a problem hiding this comment.
[P1] Preserve the list/remote-name lookup contract
On a generation mismatch this returns the completed list while deliberately leaving namesCache empty. In mapped/lowercased catalogs, buildDbForInit and buildTableForInit enumerate through listNames() and then immediately call getRemoteName(), which does requireNonNull(namesCache.getIfPresent("")). A concurrent invalidate/reset can therefore turn the newly supported "return but do not publish" case into an NPE that escapes the object-cache loader even when the requested name was in the returned list. Please have remote-name resolution share the generation-aware acquisition/result, or pass the loaded pairs through instead of assuming every returned list was cached; add a mapped-name test for this sequence.
| return Objects.requireNonNull(namesCacheLoader.load("")); | ||
| } catch (RuntimeException e) { | ||
| throw e; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
[P2] Restore the interrupt before wrapping it
The replaced Caffeine 2.9.3 loader adapter catches InterruptedException separately, calls Thread.currentThread().interrupt(), and then throws CompletionException. Here it falls into the generic Exception catch, so an interruptible CacheLoader loses its cancellation signal after this change. Please mirror the dedicated interrupted-exception handling and add a focused assertion for the flag.
| (key, value, cause) -> { }); | ||
|
|
||
| Future<List<String>> firstLoad = callers.submit(cache::listNames); | ||
| Assert.assertTrue(firstLoadStarted.await(3, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
[P2] Cover the initial wait with executor cleanup
The first submitted load and this await occur before the try/finally. If startup times out or the test thread is interrupted here, releaseFirstLoad is never released and neither executor is shut down, leaving a non-daemon fixed-pool worker blocked in the loader. Please begin the cleanup scope before submitting/awaiting the first load (and ideally await termination) so failure paths cannot strand the test JVM.
### What problem does this PR solve? Issue Number: None Related PR: apache#65126 Problem Summary: A slow external database or table name load can race with explicit metadata updates or REFRESH CATALOG on branch-4.1. The old loading-cache path could wait while holding a Caffeine key lock, publish a stale pre-invalidation list afterward, or update the names cache without atomically publishing its case-insensitive lookup map. Load remote names outside the mutation lock, publish a result only when its exact generation is still current, and keep empty-cache additions in an incomplete single-entry overlay that forces the next full load. Manual refresh uses the same generation fence, and rejected background refresh submissions keep the current value available and retryable. ### Release note Prevent stale external database or table names from being republished after concurrent metadata cache invalidation or updates. ### Check List (For Author) - Test: Unit Test - MetaCacheTest - MetaCacheDeadlockTest - FE Checkstyle - Behavior changed: Yes. Stale in-flight name loads are rejected after concurrent invalidation or explicit mutation, and case-insensitive name maps are published with the accepted names snapshot. - Does this need documentation: No
40e0200 to
6148203
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed after three convergence rounds on exact head 6148203e685f97e843fa6365fff8ad3c3473f7a8. Requesting changes for six distinct issues.
Critical checkpoints:
- Correctness and initialization: a cold mode-2 existence probe can dereference an uninitialized database cache.
- Concurrency and lifecycle: catalog reset can separate routing-map clearing from names invalidation; foreground enumeration has unbounded retry under ordinary mutation traffic; and a post-invalidation miss can remain blocked behind obsolete automatic-refresh I/O.
- Event/cache consistency: a known cold HMS event entry is made dependent on a full listing that may fail.
- Failure containment: automatic refresh exceptions escape the shared executor task and terminate workers.
- Compatibility: the removed legacy-factory overload has only the two updated in-tree callers; no separate persisted, cross-process, or plugin ABI issue was substantiated.
- Tests: the changed tests do not cover the six production paths above. Live FE UT is currently failed, but its TeamCity log requires authentication; compile was still pending at the final sweep. No local build was run per the review prompt.
- Duplicate fencing: the six existing inline threads were treated as hard fences; incomplete-snapshot publication and missing deletion intent were not repeated.
User focus: no additional focus was provided, so the full PR and its upstream/downstream lifecycle were reviewed.
| public boolean isTableExist(String tableName) { | ||
| String remoteTblName = tableName; | ||
| if (this.isTableNamesCaseInsensitive()) { | ||
| metaCache.listNames(); |
There was a problem hiding this comment.
[P1] Initialize the database before reading its names cache
On a cold object-cache miss, a database returned by ExternalCatalog.getDbNullable() is newly constructed but has not run its own makeSureInitialized(), so its metaCache is still null. CreateTableCommand.targetTableExists() immediately calls the generic DatabaseIf.isTableExist() path; with lower_case_table_names=2, this new call therefore throws before reaching the connector. Please establish database initialization (and handle initialization failure) before using metaCache, and cover a cold mode-2 existence probe.
| try { | ||
| getFilteredDatabaseNames(); | ||
| finalName = lowerCaseToDatabaseName.get(dbName.toLowerCase()); | ||
| metaCache.listNames(); |
There was a problem hiding this comment.
[P1] Keep the routing-map clear atomic with names invalidation
resetToUninitialized() clears lowerCaseToDatabaseName under the catalog monitor, releases that monitor, and only afterward invalidates metaCache through onRefreshCache(). A concurrent getDbNullable() can reinitialize in that gap; this call then hits the still-complete old names entry without republishing the cleared map, so a following differently-cased mode-2 lookup misses. This is separate from the stale-loader thread because no load runs in this interleaving. Please order the map clear and names invalidation under the same initialization fence (or republish one atomic snapshot), and add a paused reset test.
| NamesCacheValue value = namesCache.getIfPresent(""); | ||
| if (value == null || !value.complete) { | ||
| value = loadNames(false); | ||
| if (value == null) { |
There was a problem hiding this comment.
[P1] Bound retries when metadata keeps mutating
Every updateCache() or invalidate() advances the cache-wide generation, and any advance during a slow enumeration makes loadNames() return null to this unconditional loop. With even one unrelated HMS event spanning each enumeration, a foreground listNames() can issue remote loads forever and never return; the old same-key Caffeine ordering let the load finish before applying the mutation. Please replay a bounded mutation journal (including deletions) into a completed snapshot, or otherwise provide a bounded convergence/fallback policy, and test several consecutive generation changes.
| } | ||
| if (this.isTableNamesCaseInsensitive()) { | ||
| if (!isReplay) { | ||
| metaCache.listNames(); |
There was a problem hiding this comment.
[P2] Use the known event entry before requiring a full listing
On a cold mode-2 cache, registerTable() has already installed the table object and its lowercase routing entry, but HMSExternalDatabase.registerTable() immediately calls getTableNullable() to apply the event update time. This unconditional listNames() sees the incomplete entry and forces a full remote enumeration first; if that enumeration fails, the authoritative event itself fails even though the requested table is already cached. The base path consulted the specific routing entry before listing. Please allow a known incremental hit here and enumerate only on a miss, with a cold registration/loader-failure test.
| } | ||
|
|
||
| private NamesCacheValue loadNames(boolean forceRefresh) { | ||
| synchronized (namesLoadLock) { |
There was a problem hiding this comment.
[P1] Let the post-invalidation generation load independently
namesLoadLock is held across connector I/O for automatic refreshes. If one starts before invalidateAll(), invalidation advances the generation and returns, but the next listNames() blocks here behind that obsolete refresh; if its RPC hangs, refresh can never restore metadata access. The base Caffeine refresh ran asynchronously, so an invalidated synchronous miss could load independently. Please deduplicate loads per generation (or otherwise cancel/abandon the obsolete generation), and test a second lookup while a pre-invalidation automatic refresh remains paused.
| try { | ||
| namesRefreshExecutor.execute(() -> { | ||
| try { | ||
| loadNames(true); |
There was a problem hiding this comment.
[P2] Contain refresh failures inside the shared executor task
loadNames(true) can throw a runtime or wrapped checked exception, but this raw execute task only has finally, so a normal connector refresh failure escapes and terminates a worker in the shared commonRefreshExecutor. Since the old value remains refresh-eligible, repeated accesses during an outage can repeatedly kill and replace workers shared by the legacy catalog/database caches and engine metadata caches. Please catch and log loader failures inside the runnable while clearing the flag and retaining the old value, and test that a failed refresh is retried without an uncaught task failure.
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: #65126
Problem Summary:
On branch-4.1, a slow external database or table-name load can start before
REFRESH CATALOGinvalidates the legacyMetaCache. The invalidation itself returns without waiting for remote I/O, but the pre-refresh Caffeine miss load can complete afterward and publish its old names into the now-invalidated cache. A later lookup then reads metadata collected before the refresh.This is the branch-4.1 equivalent of the table-names non-blocking refresh sequence covered by #65126. It is separate from the already-fixed bounded-executor/removal-listener deadlock: the existing
MetaCacheDeadlockTestpasses on the current branch and fails only when the synchronous removal listener fix is reverted.The fix is deliberately limited to the legacy
MetaCacheused by branch-4.1. It does not backport theMetaCacheEntrycatalog/database refactor from #65126. Name misses are loaded outside Caffeine synchronized miss publication and deduplicated with one load lock. A generation and a short mutation/publication lock prevent a load that started before invalidation, update, or per-name removal from being cached afterward. Remote connector I/O remains outside the mutation lock, so refresh does not wait for the slow load.Before the fix, the deterministic test fails with:
After the fix, the first caller still receives its completed load, while the next lookup reloads and receives
local-2.Release note
Prevent stale external database or table names from being republished after concurrent metadata cache invalidation.
Check List (For Author)
MetaCacheTest: 13 tests passedMetaCacheDeadlockTest: passed./build.sh --fewas attempted, but the isolated worktree does not contain the completethirdparty/installedtree and the script started downloading/building the full third-party toolchain. It was stopped as an environment setup operation. The focused FE unit-test runs compiled all FE main sources and test sources successfully.