wenzhenghu commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3591757397
##########
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:
Thank you. Fixed in commit `4c06c0f6592` by adapting the mutating caller
instead of changing `getDbNames()` back to a mutable result.
`CatalogIf.getDbNames()` now explicitly documents that it returns a
read-only snapshot, and `MetaInfoAction` creates a mutable `ArrayList` before
sorting. I also audited `MetaInfoActionV2`: it only iterates the returned
snapshot and sorts its own `filteredDbNames` list, so no production change is
needed there.
This keeps the cache snapshot immutable and limits the O(n) copy to the
caller that actually mutates the list. The REST metadata path was also covered
by manual validation.
##########
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:
Thank you for flagging this path. After tracing the concrete HMS event flow,
the exact `CREATE_DATABASE` scenario described here is not reachable in the
standard path: `MetastoreEvent` normalizes `event.getDbName()` with
`toLowerCase(Locale.ROOT)`, and `CreateDatabaseEvent` forwards that canonical
lower-case name to `registerExternalDatabaseFromEvent()`.
The review did expose a related real gap in the cold object-load path. With
`lower_case_database_names=1`, `buildDbForInit()` could start with the
canonical local name `mixeddb` and `remoteDbName == null`, but previously did
not resolve the original remote name `MixedDb`. Commit `4c06c0f6592` now
includes mode 1 in the remote-name resolution condition and consistently uses
`Locale.ROOT`. The added FE unit test verifies that the cached database keeps
local name `mixeddb` and remote name `MixedDb`.
The broader pre-existing ALTER/DROP event API issue, where some paths mix
remote and local database-name semantics, is intentionally outside this focused
PR and should be handled separately.
##########
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:
Thank you. Fixed in commit `4c06c0f6592`.
`CatalogMgr.registerExternalTableFromEvent()` now converts the remote HMS
table name to the canonical local name before generating the table ID and
before constructing the `ExternalTable`. It calls
`buildTableForInit(remoteTableName, localTableName, ...)`, so the object name
and `NameMapping.localTblName` are canonical from construction time.
`ExternalDatabase.registerTable()` is now object-driven and publishes the
entry with `table.getRemoteName()` and `table.getName()`. The names snapshot,
object-cache key, ID mapping, and table-level metadata-cache key therefore use
one consistent remote/local pair. The same canonicalization helper is reused by
the names loader, and case conversion uses `Locale.ROOT`.
The added FE unit test covers a mapped-name event object and verifies that
the names entry, object entry, and ID mapping all use the canonical local name
without applying the mapping twice.
--
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]
