github-actions[bot] commented on code in PR #66717:
URL: https://github.com/apache/doris/pull/66717#discussion_r3826839092
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -147,7 +147,7 @@ private void cleanupRemovedCatalog(RemovedCatalog
removedCatalog) {
if (ctx != null) {
ctx.removeLastDBOfCatalog(removedCatalog.catalogName);
}
-
Env.getCurrentEnv().getExtMetaCacheMgr().removeCatalog(removedCatalog.catalogId);
+
Env.getCurrentEnv().getExtMetaCacheMgr().removeCatalogPermanently(removedCatalog.catalogId);
Review Comment:
[P1] Keep Hive generations across catalog rename
This helper is also called by `alterCatalogName()` and
`replayAlterCatalogName()`, which re-add the same catalog object and ID after
cleanup. Calling the new permanent-drop hook here removes Hive's file
invalidation/value generation maps, so lazy reinitialization restarts them at
`(0, 1)`. A statement that planned one alias before a concurrent rename can
then build the same `HiveFileScanTaskCacheKey` for another alias afterward and
reuse stale file tasks. Please invoke permanent cleanup only from
DROP/replay-DROP and retain the monotonic counters across both rename paths.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1367,9 +1384,31 @@ public int hashCode() {
public void notifyPropertiesUpdated(Map<String, String> updatedProps) {
CatalogIf.super.notifyPropertiesUpdated(updatedProps);
String schemaCacheTtl =
updatedProps.getOrDefault(SCHEMA_CACHE_TTL_SECOND, null);
- if (java.util.Objects.nonNull(schemaCacheTtl)) {
- ExternalMetaCacheMgr extMetaCacheMgr =
Env.getCurrentEnv().getExtMetaCacheMgr();
+ ExternalMetaCacheMgr extMetaCacheMgr =
Env.getCurrentEnv().getExtMetaCacheMgr();
+ if (java.util.Objects.nonNull(schemaCacheTtl)
+ ||
updatedProps.containsKey(ExternalMetaCacheBudgetManager.CATALOG_MAX_WEIGHT_PROPERTY))
{
extMetaCacheMgr.removeCatalog(id);
Review Comment:
[P2] Fence property publication with cache-group handoff
The live `CatalogProperty` is mutated before this lifecycle-fenced removal
runs (and legacy validators expose the tentative map during
`checkProperties()`). For a routed catalog with a Hive group already owning the
catalog bucket at L1, a concurrent first Iceberg/Hudi use can therefore
snapshot L2 and reach `createEntryBudget()`, which throws `Conflicting catalog
cache max weight` because the L1 group is still present. Please publish the
committed property generation and retire affected groups under the same
per-catalog fence, and cover a successful HMS ALTER racing first use of another
routed engine.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/AbstractExternalMetaCache.java:
##########
@@ -283,23 +403,53 @@ private <K, V> void
invalidateEntryIfMatched(CatalogEntryGroup group, MetaCacheE
}
}
- private CatalogEntryGroup buildCatalogEntryGroup(Map<String, String>
catalogProperties) {
+ private CatalogEntryGroup buildCatalogEntryGroup(long catalogId,
Map<String, String> catalogProperties) {
CatalogEntryGroup group = new CatalogEntryGroup();
- metaCacheEntryDefs.values()
- .forEach(entryDef -> group.put(entryDef.getName(),
newMetaCacheEntry(entryDef, catalogProperties)));
- return group;
+ try {
+ metaCacheEntryDefs.values().forEach(entryDef -> group.put(
+ entryDef.getName(), newMetaCacheEntry(catalogId, entryDef,
catalogProperties)));
+ return group;
+ } catch (RuntimeException | Error e) {
+ group.close();
+ throw e;
+ }
}
@SuppressWarnings("unchecked")
private <K, V> MetaCacheEntry<K, V> newMetaCacheEntry(
- MetaCacheEntryDef<?, ?> rawEntryDef, Map<String, String>
catalogProperties) {
+ long catalogId, MetaCacheEntryDef<?, ?> rawEntryDef, Map<String,
String> catalogProperties) {
MetaCacheEntryDef<K, V> entryDef = (MetaCacheEntryDef<K, V>)
rawEntryDef;
CacheSpec cacheSpec = CacheSpec.fromProperties(
catalogProperties, engine, entryDef.getName(),
entryDef.getDefaultCacheSpec());
- return new MetaCacheEntry<>(entryDef.getName(),
- wrapSchemaValidator(entryDef.getLoader(),
entryDef.getValueType()),
- cacheSpec,
- refreshExecutor, entryDef.isAutoRefresh(),
entryDef.isContextualOnly());
+ OptionalLong catalogMaxWeight =
budgetManager.parseCatalogMaxWeight(catalogProperties);
+ if (cacheSpec.isWeightBounded() && entryDef.getSizeEstimator() ==
null) {
+ throw new IllegalArgumentException(String.format(
+ "Entry '%s' for engine '%s' configures max-weight but has
no estimator.",
+ entryDef.getName(), engine));
+ }
+ boolean enableWeight = entryDef.getSizeEstimator() != null
Review Comment:
[P2] Include count-only entries in parent weight quotas
Requiring an entry estimator here silently excludes unestimated entries even
when `meta.cache.max-weight` or the FE-wide limit is configured. Paimon's live
schema projection retains converted `Column`/nested `Type` plus `TableSchema`
bytes without a reservation; Iceberg schemas have no admitted-parent removal
listener, and Iceberg views are parentless, so those graphs can also remain
after base reservations are released. Please provide bounded estimators for
these entries or fail closed/disable them whenever a parent weight bound
applies, with a regression proving a large schema remains charged.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/ExternalMetaCacheBudgetManager.java:
##########
@@ -0,0 +1,620 @@
+// 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.Config;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.OptionalLong;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.LongUnaryOperator;
+import java.util.stream.Collectors;
+
+/**
+ * FE-wide admission accounting for managed external metadata caches.
+ *
+ * <p>All changes are serialized by one short critical section. Cache loads and
+ * estimators run outside it, so the lock only protects a few arithmetic and
map
+ * operations while making global/catalog/entry reservation atomic.
+ */
+public final class ExternalMetaCacheBudgetManager {
+ private static final Logger LOG =
LogManager.getLogger(ExternalMetaCacheBudgetManager.class);
+ private static final ExecutorService PEER_RECLAIM_EXECUTOR =
Executors.newSingleThreadExecutor(runnable -> {
+ Thread thread = new Thread(runnable,
"external-meta-cache-peer-reclaim");
+ thread.setDaemon(true);
+ return thread;
+ });
+
+ public static final String CATALOG_MAX_WEIGHT_PROPERTY =
"meta.cache.max-weight";
+
+ private final Object lock = new Object();
+ private final OptionalLong globalMaxWeight;
+ private final Map<Long, Bucket> catalogBuckets = new HashMap<>();
+ private final Map<EntryScope, Bucket> entryBuckets = new HashMap<>();
+ private final Map<EntryScope, EntryBudget> entryBudgets = new HashMap<>();
+ private long globalUsedWeight;
+ private final AtomicLong globalRejectedCount = new AtomicLong();
+
+ public ExternalMetaCacheBudgetManager(OptionalLong globalMaxWeight) {
+ this.globalMaxWeight = Objects.requireNonNull(globalMaxWeight,
"globalMaxWeight");
+ if (globalMaxWeight.isPresent() && globalMaxWeight.getAsLong() <= 0) {
+ throw new IllegalArgumentException("global max weight must be
positive when enabled");
+ }
+ }
+
+ /**
+ * Whether any Doris meta cache weight bound (global config, catalog level
or entry level)
+ * applies to a catalog with the given properties. Used to decide whether
overlapping SDK-side
+ * metadata caches should stay enabled; parse failures count as ungoverned.
+ */
+ public static boolean appliesWeightGovernance(Map<String, String>
catalogProperties) {
+ try {
+ if (fromConfig().getGlobalMaxWeight().isPresent()) {
+ return true;
+ }
+ } catch (RuntimeException e) {
+ // An unparsable global config cannot create budgets either.
+ }
+ if (catalogProperties == null) {
+ return false;
+ }
+ if (catalogProperties.containsKey(CATALOG_MAX_WEIGHT_PROPERTY)) {
Review Comment:
[P2] Base SDK cache defaults on effective weight properties
This key-presence check contradicts the method's parse-failure contract.
Image/replay may retain an invalid catalog weight or an
obsolete/unknown/foreign-engine `*.max-weight`; Doris later sanitizes that key
away and creates no budget, but Paimon still injects `cache-enabled=false` and
Iceberg stops auto-enabling its IO manifest cache from this raw map. Those
catalogs pay recurring remote metadata work after every restart despite being
count-governed. Please derive this decision from the same parsed, routed
runtime-effective property view, while preserving the conservative result for
valid zero-weight bounds.
--
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]
