github-actions[bot] commented on code in PR #66717:
URL: https://github.com/apache/doris/pull/66717#discussion_r3836799898
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/AbstractExternalMetaCache.java:
##########
@@ -222,8 +330,35 @@ protected final ExternalTable
findExternalTable(NameMapping nameMapping, String
nameMapping.getLocalTblName(), engineNameForError));
}
+ // A contended cache-policy handoff resolves within this window; see
requireCatalogEntryGroup.
+ private static final long PREPARE_RETRY_WINDOW_NANOS = 2_000_000_000L;
+ private static final long PREPARE_RETRY_SLEEP_MS = 50L;
+
private CatalogEntryGroup requireCatalogEntryGroup(long catalogId) {
CatalogEntryGroup group = catalogEntries.get(catalogId);
+ if (group == null && catalogPreparer != null) {
+ // The caller prepared the catalog before capturing this engine,
but a cache-policy
+ // ALTER retired the group in between. Re-prepare under the
lifecycle fence so the
+ // lookup observes the new policy instead of failing a valid
catalog. The preparer
+ // never blocks on the fence (a nested default loader may hold a
Caffeine bin lock
+ // that retirement itself needs), so a contended handoff is
absorbed with a bounded
+ // sleep-and-retry: the ALTER finishes within the window, or the
lookup fails as
+ // before without any deadlock.
+ long deadlineNanos = System.nanoTime() +
PREPARE_RETRY_WINDOW_NANOS;
+ while (true) {
+ catalogPreparer.accept(catalogId);
Review Comment:
[P2] Stop retrying once the catalog is permanently gone
After `DROP CATALOG`, the preparer can acquire the lifecycle stripe and
confirm that no catalog remains, but this void callback cannot distinguish that
terminal state from lock contention. A retained-table/schema lookup therefore
sleeps in this loop for the full two-second window (about forty retries) before
`getSchemaCacheValue()` can return its intended `Optional.empty()`. This is
distinct from the existing contended-ALTER thread. Note that `ALTER CATALOG ...
RENAME` also creates a transient map absence, so treating every null lookup as
terminal would regress that handoff; please propagate a
permanent-drop/tombstone outcome (while preserving retry for rename/contended
cases) and cover both cases with latency-bounded tests.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -216,27 +472,85 @@ private SchemaCacheValue
loadSchemaCacheValue(IcebergSchemaCacheKey key) {
key.getNameMapping().getLocalTblName(),
key.getSchemaId()));
}
- private IcebergSnapshotCacheValue loadSnapshotProjection(ExternalTable
dorisTable, Table icebergTable) {
+ private SchemaCacheValue loadSchemaCacheValue(IcebergSchemaCacheKey key,
Table retainedTable) {
+ ExternalTable dorisTable = findExternalTable(key.getNameMapping(),
ENGINE);
+ dorisTable.setUpdateTime(System.currentTimeMillis());
+ boolean isView = dorisTable instanceof IcebergExternalTable
+ && ((IcebergExternalTable) dorisTable).isView();
+ SchemaCacheValue value = IcebergUtils.loadSchemaCacheValue(
+ dorisTable, key.getSchemaId(), isView,
retainedTable).orElseThrow(() ->
+ new CacheException("failed to load iceberg schema cache value
for: %s.%s.%s, schemaId: %s",
+ null, key.getNameMapping().getCtlId(),
key.getNameMapping().getLocalDbName(),
+ key.getNameMapping().getLocalTblName(),
key.getSchemaId()));
+ // Contextual miss loaders bypass the default-loader schema validator;
ambiguous
+ // case-insensitive column names must be rejected on this path too.
+ value.validateSchema();
+ return value;
+ }
+
+ private void retireTableGeneration(NameMapping nameMapping,
+ @Nullable IcebergTableCacheValue previousValue,
IcebergTableCacheValue currentValue) {
+ if (previousValue != null &&
previousValue.isSameOperationalGeneration(currentValue)) {
Review Comment:
[P1] Retire projections when the authenticator changes
An auth-only catalog ALTER resets A1 to A2 without retiring the Iceberg
cache group. After the table entry refreshes the same UUID/metadata file and
equivalent FileIO resources under A2, this equality check still returns early
because the captured authenticator is not part of the operational generation;
the hit-side `sharesOperationalResources` check has the same omission. The
snapshot therefore remains bound to A1, `IcebergScanNode` correctly rejects it
under A2, and every retried statement keeps hitting the same rejected
projection until expiry or explicit invalidation. This is the recovery path
beyond the existing scan-fence thread: failing safely is not retryable if the
stale projection survives. Please include authenticator identity in replacement
and hit revalidation (or retire the group on operational ALTER), with an A1 ->
auth-only A2 -> same-metadata refresh 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]
