github-actions[bot] commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3681246476
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -110,6 +110,13 @@ public void replayRefreshDb(ExternalObjectLog log) {
}
if (!db.isPresent()) {
+ if (!Strings.isNullOrEmpty(log.getDbName())) {
Review Comment:
[P1] Preserve ID-only replay with a cold database cache
This fallback only covers current name-bearing logs. Legacy
`ExternalObjectLog` records can contain only `dbId`/`tableId`; after this
refactor `getDbForReplay(long)` is cache-only, so a retained `dbIdToName` plus
a TTL-zero or evicted database object yields empty and both replay methods
return without invalidating independently hot engine metadata. The old
`getMetaObjById` path loaded the database on this miss, so this is a
rolling-upgrade/replay regression. Please expose the retained database name to
replay and invalidate that database scope (conservatively for an ID-only table
log) without remote I/O, with TTL-zero ID-only tests for both record types.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -599,10 +601,154 @@ private String getLocalTableName(String tableName,
boolean isReplay) {
return finalName;
}
+ private NameCacheValue getTableNamesValue(boolean allowLoad) {
+ if (tableNames == null) {
+ return null;
+ }
+ return allowLoad ? tableNames.get("") : tableNames.getIfPresent("");
+ }
+
+ // Centralize names-negative-lookup handling so all table lookup paths
share the same config-driven policy.
+ @Nullable
+ private <R> R resolveTableNameFromSnapshot(String lookupName, boolean
isReplay,
+ Function<NameCacheValue, R> resolver) {
+ NameCacheValue cached = getTableNamesValue(false);
+ if (cached == null) {
+ if (isReplay) {
+ return null;
+ }
+ NameCacheValue loaded = getTableNamesValue(true);
+ return loaded == null ? null : resolver.apply(loaded);
+ }
+ R resolved = resolver.apply(cached);
+ if (resolved != null || isReplay ||
!Config.enable_external_meta_cache_name_miss_refresh) {
+ return resolved;
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("refresh table names after hot-snapshot miss, catalog:
{}, db: {}, lookup: {}",
+ getCatalog().getName(), getFullName(), lookupName);
+ }
+ resetMetaCacheNames();
+ NameCacheValue refreshed = getTableNamesValue(true);
+ return refreshed == null ? null : resolver.apply(refreshed);
+ }
+
+ private List<String> listLocalTableNamesFromCache() {
+ NameCacheValue namesValue = java.util.Objects.requireNonNull(
+ getTableNamesValue(true), "table names cache can not be null");
+ return namesValue.localNames();
+ }
+
+ private List<String> listLocalTableNamesWithoutCache(SessionContext
sessionContext) {
+ return
listTableNames(sessionContext).stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ @Nullable
+ private String getRemoteTableName(String localTableName, boolean isReplay)
{
+ // Route local-to-remote resolution through the shared helper so miss
reload stays consistent with lookups.
+ return resolveTableNameFromSnapshot(localTableName, isReplay,
+ namesValue ->
namesValue.remoteNameOfLocalName(localTableName));
+ }
+
+ private Optional<Pair<String, String>>
findTableNamePairWithoutCache(SessionContext sessionContext,
+ String requestedTableName) {
+ return listTableNames(sessionContext).stream()
+ .filter(pair -> matchesLocalTableName(pair.value(),
requestedTableName))
+ .findFirst();
+ }
+
+ private boolean matchesLocalTableName(String localTableName, String
requestedTableName) {
+ if (isStoredTableNamesLowerCase()) {
+ return
localTableName.equals(requestedTableName.toLowerCase(Locale.ROOT));
+ }
+ if (isTableNamesCaseInsensitive()) {
+ return localTableName.equalsIgnoreCase(requestedTableName);
+ }
+ return localTableName.equals(requestedTableName);
+ }
+
+ private String resolveTableNameForInvalidation(String tableName) {
+ if (isStoredTableNamesLowerCase()) {
+ return tableName.toLowerCase(Locale.ROOT);
+ }
+ if (!isTableNamesCaseInsensitive()) {
+ return tableName;
+ }
+
+ String localTableName = getLocalTableName(tableName, true);
+ if (localTableName != null) {
+ return localTableName;
+ }
+ T cachedTable = tables.findIfPresent(key ->
key.equalsIgnoreCase(tableName));
+ if (cachedTable != null) {
+ return cachedTable.getName();
+ }
+ return tableIdToName.values().stream()
+ .filter(name -> name.equalsIgnoreCase(tableName))
+ .findFirst()
+ .orElse(tableName);
+ }
+
+ private void updateTableCache(T table, String remoteTableName, String
localTableName) {
+ updateTableCache(table, remoteTableName, localTableName, false);
+ }
+
+ protected void updateTableCache(T table, String remoteTableName, String
localTableName,
+ boolean forceUpdateCacheState) {
+ buildMetaCache();
+ // 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.
+ // By default, incremental updates keep cold names/object cache
entries cold.
+ // forceUpdateCacheState is only for paths that intentionally populate
those cold entries.
+ if (forceUpdateCacheState) {
+ tableNames.compute("", (ignored, current) ->
+ (current == null ? NameCacheValue.empty() :
current).withName(remoteTableName, localTableName));
+ } else {
+ // Keep a cold names entry cold, but still advance its generation
so an in-flight pre-event load cannot
+ // publish a stale snapshot after this incremental update.
+ tableNames.compute("", (ignored, current) ->
+ current == null ? null : current.withName(remoteTableName,
localTableName));
+ }
+ tables.computeAndRun(
+ localTableName,
+ (ignored, current) -> (forceUpdateCacheState || current !=
null) ? table : null,
+ () -> tableIdToName.put(table.getId(), localTableName));
+ }
+
+ protected void invalidateTableCache(String localTableName) {
+ if (tableNames != null) {
+ // Keep a cold names entry cold, but fence any in-flight pre-drop
load from publishing stale state.
+ tableNames.compute("", (ignored, current) ->
+ current == null ? null :
current.withoutLocalName(localTableName));
+ }
+ if (tables != null) {
+ tables.invalidateKeyAndRun(
+ localTableName,
+ () -> tableIdToName.entrySet().removeIf(
+ entry -> entry.getValue().equals(localTableName)));
Review Comment:
[P2] Avoid scanning all retained table IDs on every drop
`tableIdToName` intentionally outlives names/object eviction and can contain
every table successfully looked up or registered. Each targeted DROP/rename now
runs `entrySet().removeIf` over that whole map while `CatalogMgr` holds the
database write lock and `invalidateKeyAndRun` holds the object-cache stripe
monitor. A burst over N retained tables can therefore serialize toward O(N²),
whereas the deleted `MetaCache.invalidate` removed the known ID directly.
Please carry/compute the deterministic table ID or maintain a reverse index for
targeted removal, and add a large cold-index complexity guard.
--
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]
