github-actions[bot] commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3585200661
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java:
##########
@@ -211,8 +210,8 @@ public void registerDatabase(long dbId, String dbName) {
ExternalDatabase<? extends ExternalTable> db = buildDbForInit(dbName,
null, dbId, logType, false);
if (isInitialized()) {
- metaCache.updateCache(db.getRemoteName(), db.getFullName(), db,
- Util.genIdByName(name, db.getFullName()));
+ // Keep names/object/id cache updates in one helper so HMS
incremental sync follows the same ordering.
+ updateDatabaseCache(db.getRemoteName(), db.getFullName(), db);
}
Review Comment:
In `lower_case_database_names=1`, this can publish the HMS event spelling
instead of the canonical lower-case local name.
`registerExternalDatabaseFromEvent()` passes the raw HMS name into
`registerDatabase()`, and `buildDbForInit(dbName, null, ...)` only applies
`fromRemoteDatabaseName()`, not the mode-1 lower-case conversion. So a create
event for `MixedDb` records names/object/`dbIdToName` under `MixedDb`. A later
drop event calls `resolveDatabaseNameForInvalidation()`, which returns
`mixeddb` in mode 1, so `invalidateDatabaseCache("mixeddb")` misses the raw hot
entries and by-ID mapping. Please canonicalize the local DB name before
building/updating the incremental cache, or make invalidation recover and
remove the existing hot/id-map key.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1294,6 +1321,129 @@ 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.localNames();
+ }
Review Comment:
Returning `namesValue.localNames()` exposes the immutable `ImmutableList`
stored inside `NameCacheValue`, but `getDbNames()` used to return a fresh
mutable list from `MetaCache.listNames()`. Existing callers still mutate that
result; for example `MetaInfoAction` does `List<String> dbNames =
catalog.getDbNames();` and then `Collections.sort(dbNames)`, so
external-catalog metadata requests can now fail with
`UnsupportedOperationException`. Please keep the old API contract by returning
a mutable copy here, or update all callers that sort/mutate the returned list.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -655,10 +774,8 @@ public boolean registerTable(TableIf tableIf) {
LOG.debug("create table [{}]", tableName);
}
if (isInitialized()) {
- String localName = extCatalog.fromRemoteTableName(this.remoteName,
tableName);
- metaCache.updateCache(tableName, localName, (T) tableIf,
- Util.genIdByName(extCatalog.getName(), name, localName));
- lowerCaseToTableName.put(tableName.toLowerCase(), tableName);
+ String localName = toLocalTableName(tableName);
+ updateTableCache((T) tableIf, tableName, localName);
}
Review Comment:
This normalizes only the cache key, not the `ExternalTable` object built
from HMS events. `registerExternalTableFromEvent()` constructs the table with
`buildTableForInit(tableName, tableName, ...)`, so in
`lower_case_table_names=1` an event for `Foo` creates an `ExternalTable` whose
`name` and `NameMapping.localTblName` are still `Foo`; then this code stores it
under the lower-case key `foo`. If that hot object initializes schema or
partition caches, those entries are keyed by `Foo`, but `unregisterTable()`
later invalidates `foo` and `AbstractExternalMetaCache.matchTable()` uses exact
local-name equality. Please normalize the local table name before
constructing/publishing the event table, or update the table object/NameMapping
so object state and cache keys use the same canonical local name.
--
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]
