github-actions[bot] commented on code in PR #66913:
URL: https://github.com/apache/doris/pull/66913#discussion_r3838824874
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -107,7 +127,13 @@ public MetaCacheEntry(String name, @Nullable Function<K,
V> loader, CacheSpec ca
maxSize,
true,
null);
- this.loadingData =
cacheFactory.buildCache(this::loadFromDefaultLoader, refreshExecutor);
+ if (retirementListener != null) {
+ this.loadingData = cacheFactory.buildCacheWithSyncRemovalListener(
Review Comment:
[P1] Keep Iceberg refresh off the query thread
`buildCacheWithSyncRemovalListener()` replaces the supplied
`refreshExecutor` with `Runnable::run`, and Caffeine uses the builder executor
for refresh-after-write as well as listener delivery. The Iceberg table entry
combines `autoRefresh=true` with a retirement listener, so its first stale hit
now runs `loadTableCacheValue()`—including remote
catalog/table/FileIO/credential loading—inline before `get()` can return the
old value. A slow or unavailable catalog can therefore stall hot-cache queries
every refresh interval. Preserve asynchronous refresh dispatch while making
retirement non-droppable, and add a blocked-reload test proving a stale hit
returns the old value before reload completes.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java:
##########
@@ -173,17 +188,95 @@ protected List<String>
listTableNamesFromRemote(SessionContext ctx, String dbNam
}
@Override
- public void onClose() {
+ public synchronized void onClose() {
+ ThreadPoolExecutor retiredExecutor = threadPoolWithPreAuth;
+ threadPoolWithPreAuth = null;
super.onClose();
- if (null != catalog) {
- try {
- if (catalog instanceof AutoCloseable) {
- ((AutoCloseable) catalog).close();
- }
- catalog = null;
- } catch (Exception e) {
- LOG.warn("Failed to close iceberg catalog: {}", getName(), e);
+ Catalog retiredCatalog = catalog;
+ catalog = null;
+ resourceTracker.retireCurrent(() -> {
+ closeCatalog(retiredCatalog);
+ if (retiredExecutor != null) {
+ ThreadPoolManager.shutdownExecutorService(retiredExecutor);
}
+ });
+ }
+
+ @Override
+ public synchronized void resetToUninitialized(boolean invalidCache) {
+ ExternalMetaCacheMgr cacheMgr =
Env.getCurrentEnv().getExtMetaCacheMgr();
+ cacheMgr.runCatalogLifecycle(getId(), () ->
resetCatalogRuntime(cacheMgr, invalidCache));
+ }
+
+ private void resetCatalogRuntime(ExternalMetaCacheMgr cacheMgr, boolean
invalidCache) {
+ cacheMgr.removeCatalogByEngine(getId(),
IcebergExternalMetaCache.ENGINE);
+ super.resetToUninitialized(invalidCache);
+ }
+
+ private void closeCatalog(Catalog retiredCatalog) {
+ if (retiredCatalog == null) {
+ return;
+ }
+ try {
+ if (retiredCatalog instanceof AutoCloseable) {
+ ((AutoCloseable) retiredCatalog).close();
Review Comment:
[P1] Actually close HiveCatalog's shared FileIO
For a native Iceberg-HMS catalog this invokes the inherited
`BaseMetastoreCatalog.close()`, but Iceberg 1.10.1 `HiveCatalog` does not
override it and the base implementation closes only the metrics reporter, not
the per-catalog `fileIO`. The `type=hms` Iceberg fallback is even weaker: its
tracker callback calls `IcebergMetadataOps.close()`, which only nulls the
wrapped catalog reference. Because table-level cleanup intentionally skips HMS
shared IO, repeated resets can abandon every generation's closeable custom
FileIO after its last lease closes. Give each HiveCatalog generation an
idempotent owner that actually closes that FileIO from the tracker callback,
and cover both construction paths with repeated-reset tests.
--
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]
