wenzhenghu commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3542200255
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1281,6 +1297,103 @@ private String getLocalDatabaseName(String dbName,
boolean isReplay) {
return finalName;
}
+ private NameCacheValue getDatabaseNamesValue(boolean allowLoad) {
+ if (databaseNames == null) {
+ return null;
+ }
+ return allowLoad ? databaseNames.get("") :
databaseNames.getIfPresent("");
+ }
+
+ // Centralize names-negative-lookup handling so all database lookup paths
share the same config-driven policy.
+ @Nullable
+ private <T> T resolveDatabaseNameFromSnapshot(String lookupName, boolean
isReplay,
+ Function<NameCacheValue, T> resolver) {
+ NameCacheValue cached = getDatabaseNamesValue(false);
+ if (cached == null) {
+ if (isReplay) {
+ return null;
+ }
+ NameCacheValue loaded = getDatabaseNamesValue(true);
+ return loaded == null ? null : resolver.apply(loaded);
+ }
+ T resolved = resolver.apply(cached);
+ if (resolved != null || isReplay ||
!Config.enable_external_meta_cache_name_miss_refresh) {
+ return resolved;
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("refresh database names after hot-snapshot miss,
catalog: {}, lookup: {}",
+ getName(), lookupName);
+ }
+ invalidateDatabaseNamesOnly();
+ NameCacheValue refreshed = getDatabaseNamesValue(true);
+ return refreshed == null ? null : resolver.apply(refreshed);
+ }
+
+ private List<String> listLocalDatabaseNamesFromCache() {
+ NameCacheValue namesValue = java.util.Objects.requireNonNull(
+ getDatabaseNamesValue(true), "database names cache can not be
null");
+ return
namesValue.names().stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ @Nullable
+ protected String getRemoteDatabaseName(String localDbName, boolean
isReplay) {
+ // Route local-to-remote resolution through the shared helper so miss
reload stays consistent with lookups.
+ return resolveDatabaseNameFromSnapshot(localDbName, isReplay,
+ namesValue -> namesValue.remoteNameOfLocalName(localDbName));
+ }
+
+ protected final void updateDatabaseCache(String remoteDbName, String
localDbName,
+ ExternalDatabase<? extends ExternalTable> db) {
+ updateDatabaseCache(remoteDbName, localDbName, db, false);
+ }
+
+ private void updateDatabaseCache(String remoteDbName, String localDbName,
+ ExternalDatabase<? extends ExternalTable> db, boolean
forceUpdateCacheState) {
+ buildMetaCache();
+ long dbId = db.getId();
+ // Runtime incremental events only maintain names and object entries
that are already hot. The ID map is a
+ // lightweight lookup index and must always track registered objects
so normal by-ID lookup can load on demand.
+ if (forceUpdateCacheState) {
+ databaseNames.compute("", (ignored, current) ->
+ (current == null ? NameCacheValue.empty() :
current).withName(remoteDbName, localDbName));
+ } else if (databaseNames.getIfPresent("") != null) {
+ databaseNames.compute("", (ignored, current) ->
+ current == null ? null : current.withName(remoteDbName,
localDbName));
+ }
+ if (forceUpdateCacheState || databases.getIfPresent(localDbName) !=
null) {
+ databases.put(localDbName, db);
Review Comment:
Thank you for pointing out this race window.
I agree that `getIfPresent("")` and the following `compute("", ...)` are not
atomic, so `current` may still become `null` inside `compute(...)` if another
thread invalidates the names entry in between.
After revisiting this path, my understanding is that the consequence here is
a performance corner case rather than a correctness issue. If the entry has
already been invalidated concurrently, keeping the names cache cold is still
acceptable, and a later reader will reload the authoritative snapshot on demand.
I also do not think removing the outer `getIfPresent` guard would materially
change the behavior, because the `current == null ? null : ...` branch already
preserves the cold-entry-skip semantics. So for now I would prefer to keep the
logic unchanged in this PR.
If helpful, I can add a brief comment to make it clearer that `current` may
still be `null` inside `compute(...)` despite the outer hot-entry check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
