github-actions[bot] commented on code in PR #67417:
URL: https://github.com/apache/doris/pull/67417#discussion_r3933254439
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -430,13 +445,15 @@ public DatabaseProperty getDbProperties() {
@Override
public boolean isTableExist(String tableName) {
+ makeSureInitialized();
+ if (!isInitialized()) {
+ throw new IllegalStateException("Failed to initialize external
database " + getFullName());
+ }
String remoteTblName = tableName;
if (this.isTableNamesCaseInsensitive()) {
remoteTblName = lowerCaseToTableName.get(tableName.toLowerCase());
if (remoteTblName == null) {
- // Here we need to execute listTableNames() once to fill in
lowerCaseToTableName
- // to prevent lowerCaseToTableName from being empty in some
cases
- listTableNames();
+ metaCache.listNames();
Review Comment:
[P1] Force a refresh for a case-insensitive routing miss
On a warm complete snapshot, `metaCache.listNames()` returns the cached
names immediately; at most it schedules an eligible refresh asynchronously. If
a table was added directly in the remote system and has no event overlay, this
miss therefore checks the same stale lowercase map again and returns `false`,
whereas the replaced `listTableNames()` call synchronously discovered it. The
same regression affects `getLocalTableName()` and the catalog-level mode-2
fallback. Please provide a generation/epoch-fenced synchronous force-load for
non-replay misses and add a warm-cache, out-of-band mixed-case creation test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -127,26 +438,28 @@ public Optional<T> getMetaObjById(long id) {
public void updateCache(String remoteName, String localName, T obj, long
id) {
metaObjCache.put(localName, Optional.of(obj));
- namesCache.asMap().compute("", (k, v) -> {
- if (v == null) {
- return Lists.newArrayList(Pair.of(remoteName, localName));
- } else {
- v.add(Pair.of(remoteName, localName));
- return v;
- }
- });
+ synchronized (namesMutationLock) {
+ long generation = advanceNamesGeneration();
Review Comment:
[P1] Fence event mutations across catalog reset
A create event can build its object with the old mapping/filter, pass the
caller's `isInitialized()` check, and pause before `updateCache()`. Reset now
performs its only names invalidation before `onClose()`, so that event can
resume afterward and this method installs the old pair in the new generation
without consulting `metadataLoadEpoch`. The next current-property full load
then overlays this pair, which can resurrect an excluded or pre-remap name even
though object invalidation runs later. Please validate an event's lifecycle
token (or serialize its admission/publication with reset) and test an event
paused across ALTER/reset.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +160,249 @@ public MetaCache(String name,
maxSize,
true,
null);
- namesCache = namesCacheFactory.buildCache(namesCacheLoader, executor);
+ namesCache = namesCacheFactory.buildCache();
// Use sync removal listener to prevent deadlock (removal listener
calls invalidateAll)
// NOTE: This cache should NOT use refreshAfterWrite, as it would
become synchronous
metaObjCache =
objCacheFactory.buildCacheWithSyncRemovalListener(metaObjCacheLoader,
removalListener);
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ return
getNames().stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ private List<Pair<String, String>> getNames() {
+ for (int attempt = 0; attempt < MAX_NAMES_LOAD_ATTEMPTS; attempt++) {
+ NamesCacheValue value = namesCache.getIfPresent("");
+ List<Pair<String, String>> currentNames = null;
+ synchronized (namesMutationLock) {
+ if (value != null && value.complete && value.generation ==
namesGeneration.get()) {
+ currentNames = value.snapshot();
+ }
+ }
+ if (currentNames != null) {
+ scheduleNamesRefresh(value);
+ return currentNames;
+ }
+ value = loadNames(false, true, null);
+ synchronized (namesMutationLock) {
+ if (value != null && value.complete && value.generation ==
namesGeneration.get()) {
+ return value.snapshot();
+ }
+ }
+ }
+ throw new IllegalStateException("Failed to load names for " + name
+ + " because metadata kept changing");
+ }
+
+ private NamesCacheValue loadNames(boolean forceRefresh, boolean
awaitActiveLoad, Long expectedGeneration) {
+ NamesLoad namesLoad = null;
+ boolean loadOwner = false;
+ long requestedGeneration;
+ synchronized (namesMutationLock) {
+ if (expectedGeneration != null && expectedGeneration !=
namesGeneration.get()) {
+ return null;
+ }
+ NamesCacheValue cached = namesCache.getIfPresent("");
+ if (!forceRefresh && cached != null && cached.complete
+ && cached.generation == namesGeneration.get()) {
+ return cached;
+ }
+ long loadGeneration = namesGeneration.get();
+ if (activeNamesLoad != null && activeNamesLoad.generation ==
loadGeneration) {
+ if (!awaitActiveLoad) {
+ return null;
+ }
+ namesLoad = activeNamesLoad;
+ }
+ requestedGeneration = loadGeneration;
+ }
+
+ if (namesLoad != null) {
+ return awaitNamesLoad(namesLoad);
+ }
+
+ // Lifecycle admission may acquire the catalog monitor. Keep it
outside the names
+ // mutation lock because catalog reset advances the names generation
under that monitor.
+ long loadEpoch = namesLoadEpochSupplier.getAsLong();
+ synchronized (namesMutationLock) {
+ if (requestedGeneration != namesGeneration.get()
+ || expectedGeneration != null && expectedGeneration !=
namesGeneration.get()) {
+ return null;
+ }
+ NamesCacheValue cached = namesCache.getIfPresent("");
+ if (!forceRefresh && cached != null && cached.complete
+ && cached.generation == requestedGeneration) {
+ return cached;
+ }
+ if (activeNamesLoad != null && activeNamesLoad.generation ==
requestedGeneration) {
+ if (!awaitActiveLoad) {
+ return null;
+ }
+ namesLoad = activeNamesLoad;
+ } else {
+ if (physicalNamesLoads.size() >= MAX_PHYSICAL_NAMES_LOADS) {
+ return null;
+ }
+ Map<String, Pair<String, String>> incompleteNames = cached !=
null && !cached.complete
+ ? Maps.newLinkedHashMap(cached.names) :
Maps.newLinkedHashMap();
+ namesLoad = new NamesLoad(requestedGeneration, loadEpoch,
incompleteNames);
+ activeNamesLoad = namesLoad;
+ physicalNamesLoads.add(namesLoad);
+ loadOwner = true;
+ }
+ }
+
+ if (!loadOwner) {
+ return awaitNamesLoad(namesLoad);
+ }
+
+ try {
+ List<Pair<String, String>> loadedNames =
Objects.requireNonNull(namesCacheLoader.load(""));
+ NamesCacheValue value = null;
+ synchronized (namesMutationLock) {
+ if (namesLoad.generation == namesGeneration.get()
+ && namesLoad.generation >= minimumLoadGeneration
+ && namesLoadEpochValidator.test(namesLoad.loadEpoch)) {
+ Map<String, Pair<String, String>> names =
toNamesMap(loadedNames);
+ names.putAll(namesLoad.incompleteNames);
+ value = new NamesCacheValue(namesGeneration.get(), names,
true);
+ namesCache.put("", value);
+ publishNames(value);
+ }
+ }
+ namesLoad.result.complete(value);
+ return value;
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ CompletionException failure = new CompletionException(e);
+ namesLoad.result.completeExceptionally(failure);
+ throw failure;
+ } catch (RuntimeException e) {
+ namesLoad.result.completeExceptionally(e);
Review Comment:
[P2] Retry when this load was already retired
`advanceNamesGeneration()` can complete this owner's future with `null`
before the obsolete physical call returns. If reset then closes the old
connector and this call throws, `completeExceptionally` returns false, but this
branch still propagates the stale-client error. Joiners and an obsolete owner
that returns normally both retry the current generation, so only the failing
owner spuriously aborts even when replacement metadata is available. Please
preserve current-owner failures, but when retirement already won the future
completion, return `null` so `getNames()` follows its bounded retry path; add
an invalidate-then-old-loader-fails regression.
--
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]
