github-actions[bot] commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3602162043
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -345,28 +340,48 @@ public Optional<T> getTableForReplay(long tableId) {
if (!isInitialized()) {
return Optional.empty();
}
- return metaCache.getMetaObjById(tableId);
+ String tableName = tableIdToName.get(tableId);
+ if (tableName == null) {
+ return Optional.empty();
+ }
+ return Optional.ofNullable(tables.getIfPresent(tableName));
Review Comment:
**[P1] Preserve ID-only refresh replay after table eviction**
`RefreshManager.replayRefreshTable()` still falls back to this method for
legacy `ExternalObjectLog` records with no `tableName`. Table-object
TTL/capacity eviction leaves `tableIdToName` and the independent engine
schema/partition caches intact because this cache has no removal listener.
Returning empty here makes replay exit before `refreshTableInternal()`, so a
follower can retain stale engine metadata. Keep replay nonblocking by exposing
the retained name and invalidating engine caches by name when the object is
cold, and add an ID-only replay test after deterministic object eviction.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -650,15 +773,12 @@ public CatalogIf getCatalog() {
@Override
public boolean registerTable(TableIf tableIf) {
makeSureInitialized();
- String tableName = tableIf.getName();
+ T table = (T) tableIf;
if (LOG.isDebugEnabled()) {
- LOG.debug("create table [{}]", tableName);
+ LOG.debug("create table [{}]", table.getName());
}
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);
+ updateTableCache(table, table.getRemoteName(), table.getName());
Review Comment:
**[P1] Keep HMS CREATE events off the load-through path**
With names and objects cold, this call intentionally publishes only
`tableIdToName`, but virtual dispatch then returns to
`HMSExternalDatabase.registerTable()`, which immediately calls
`getTableNullable()`. That miss reaches `listTableNames()` and remote HMS I/O
while `CatalogMgr.registerExternalTableFromEvent()` still holds
`db.writeLock()`, so a slow metastore blocks this database and serialized event
processing; a lagging list can also discard the just-delivered object. The new
wrapper test bypasses this HMS override. Since `CatalogMgr` already sets the
event object's update time, remove the post-register load-through or use a
cache-only update, and test the real override with cold caches.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/NameCacheValue.java:
##########
@@ -0,0 +1,118 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.metacache;
+
+import org.apache.doris.common.Pair;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Immutable snapshot of remote/local names and the case-insensitive
remote-name index.
+ */
+public final class NameCacheValue {
+ private final ImmutableList<Pair<String, String>> names;
+ private final ImmutableList<String> localNames;
+ private final ImmutableMap<String, String> lowerCaseToRemoteName;
+ private final ImmutableMap<String, String> localNameToRemoteName;
+ private final ImmutableSet<String> localNameSet;
+
+ private NameCacheValue(List<Pair<String, String>> names) {
+ // Deep-copy each pair so callers cannot mutate the snapshot through
reused Pair instances.
+ this.names = ImmutableList.copyOf(copyPairs(names));
+ // Build the lower-case index with last-write-wins semantics so the
snapshot does not
+ // introduce case-conflict validation beyond what the catalog-aware
loader already enforces.
+ Map<String, String> indexBuilder = new java.util.HashMap<>();
+ Map<String, String> localNameIndexBuilder = new java.util.HashMap<>();
+ ImmutableList.Builder<String> localNamesBuilder =
ImmutableList.builder();
+ for (Pair<String, String> pair : this.names) {
+ indexBuilder.put(pair.key().toLowerCase(Locale.ROOT), pair.key());
+ localNamesBuilder.add(pair.value());
+ localNameIndexBuilder.put(pair.value(), pair.key());
+ }
+ localNames = localNamesBuilder.build();
+ lowerCaseToRemoteName = ImmutableMap.copyOf(indexBuilder);
+ localNameToRemoteName = ImmutableMap.copyOf(localNameIndexBuilder);
+ localNameSet = ImmutableSet.copyOf(localNames);
+ }
+
+ public static NameCacheValue of(List<Pair<String, String>> names) {
+ return new NameCacheValue(Objects.requireNonNull(names, "names can not
be null"));
+ }
+
+ public static NameCacheValue empty() {
+ return of(ImmutableList.of());
+ }
+
+ public List<Pair<String, String>> names() {
+ // Return fresh Pair objects even though the list itself is immutable,
so callers cannot
+ // mutate the published snapshot through reused Pair instances.
+ return ImmutableList.copyOf(copyPairs(names));
+ }
+
+ public List<String> localNames() {
+ return localNames;
+ }
+
+ public String remoteNameOfLocalName(String localName) {
+ return localNameToRemoteName.get(localName);
+ }
+
+ public boolean containsLocalName(String localName) {
+ return localNameSet.contains(localName);
+ }
+
+ public String remoteNameForCaseInsensitiveLookup(String name) {
+ return lowerCaseToRemoteName.get(name.toLowerCase(Locale.ROOT));
+ }
+
+ public NameCacheValue withName(String remoteName, String localName) {
+ for (Pair<String, String> pair : names) {
+ if (pair.key().equals(remoteName) &&
!pair.value().equals(localName)) {
+ throw new IllegalArgumentException(
+ "remote name already maps to another local name: " +
remoteName);
+ }
+ }
+ // Copy-on-write keeps readers on a stable snapshot while publishing a
new value atomically.
+ List<Pair<String, String>> copy = Lists.newArrayList(names);
Review Comment:
**[P2] Avoid rebuilding the full names index for every HMS event**
`withName()` runs inside the single-key `MetaCacheEntry.compute()`
publication lock (and the event caller's database write lock). It scans/copies
the complete list, then `of()` deep-copies every existing `Pair` again and
rebuilds three indexes. For a database with 100k tables, each CREATE therefore
allocates/copies hundreds of thousands of entries while all name events
serialize; the baseline CREATE path was O(1). Please use an incremental
snapshot representation that avoids full reconstruction and redundant pair
copies under these locks, and cover a large hot snapshot/event burst with a
benchmark or regression 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]
