github-actions[bot] commented on code in PR #66914:
URL: https://github.com/apache/doris/pull/66914#discussion_r3820163730
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTableCache.java:
##########
@@ -61,10 +62,15 @@ final class IcebergTableCache {
private final MetaCacheEntry<TableIdentifier, Table> entry;
IcebergTableCache(long ttlSeconds, int maxSize) {
+ this(ttlSeconds, maxSize, table -> { });
+ }
+
+ IcebergTableCache(long ttlSeconds, int maxSize, Consumer<Table> cleaner) {
// "<= 0 disables" connector TTL contract, folded to CacheSpec's
disable sentinel (CacheSpec.ofConnectorTtl).
CacheSpec spec = CacheSpec.ofConnectorTtl(ttlSeconds, maxSize);
this.entry = new MetaCacheEntry<>("iceberg-table", null, spec,
- ForkJoinPool.commonPool(), false, true, 0L, true);
+ ForkJoinPool.commonPool(), false, true, 0L, true,
Review Comment:
[P1] Do not treat cache removal as the last owner of this `Table`.
`MetaCacheEntry` can synchronously invoke this cleaner after
`removeLoadedValue` and then return that same loaded object; hits are also
retained by `IcebergStatementScope` and lazy split planning after the cache
lookup, so `REFRESH TABLE`, TTL, or capacity eviction can close FileIO while a
query is still reading manifests. The opposite race (invalidation before
publication), plus the supported `ttl <= 0` mode, returns a table that never
enters the cache and therefore never reaches this listener. Please introduce a
borrower/lease or statement-scoped ownership boundary that closes only after
eviction and the final user release, and test both invalidation interleavings
plus active TTL/capacity borrowers.
##########
fe/fe-connector/fe-connector-cache/src/main/java/org/apache/doris/connector/cache/MetaCacheEntry.java:
##########
@@ -84,6 +85,19 @@ public MetaCacheEntry(String name, Function<K, V> loader,
CacheSpec cacheSpec, E
public MetaCacheEntry(String name, Function<K, V> loader, CacheSpec
cacheSpec,
ExecutorService refreshExecutor, boolean autoRefresh, boolean
contextualOnly,
long refreshAfterWriteSeconds, boolean manualMissLoadEnabled) {
+ this(name, loader, cacheSpec, refreshExecutor, autoRefresh,
contextualOnly,
+ refreshAfterWriteSeconds, manualMissLoadEnabled, null);
+ }
+
+ /**
+ * Creates an entry with a synchronous removal listener. The listener is
invoked when a cached value is
+ * evicted or invalidated, allowing the cache to release resources owned
by the value (e.g. Iceberg FileIO).
+ * This variant does not use refreshAfterWrite, so synchronous removal is
safe.
+ */
+ public MetaCacheEntry(String name, Function<K, V> loader, CacheSpec
cacheSpec,
+ ExecutorService refreshExecutor, boolean autoRefresh, boolean
contextualOnly,
+ long refreshAfterWriteSeconds, boolean manualMissLoadEnabled,
Review Comment:
[P2] Keep the `MetaCacheEntry` API Caffeine-free. This module's class
javadoc and POM explicitly rely on Caffeine being encapsulated/`provided`, but
this public overload puts `RemovalListener` in the method descriptor, forcing
connector consumers to resolve an implementation-library type (Iceberg only
happens to declare Caffeine for another reason). Please accept a
JDK/Doris-owned callback such as `BiConsumer<K,V>` or a cache-owned listener
interface here and adapt it to Caffeine inside `MetaCacheEntry`/`CacheFactory`.
##########
fe/fe-connector/fe-connector-hudi/src/main/java/org/apache/doris/connector/hudi/HudiScanPlanProvider.java:
##########
@@ -294,30 +294,33 @@ basePath, inputFormat, serdeLib, columnNames,
columnTypes, partitionFieldNames(m
HoodieLocalEngineContext engineCtx = new
HoodieLocalEngineContext(metaClient.getStorageConf());
HoodieTableFileSystemView fsView =
FileSystemViewManager.createInMemoryFileSystemView(
engineCtx, metaClient, metadataConfig);
+ try {
+ // Resolve partitions
+ List<String> partitionPaths = resolvePartitions(hudiHandle,
metaClient);
Review Comment:
[P1] Close the metadata reader created while resolving partitions.
`resolvePartitions` reaches `listAllPartitionPaths`, which creates a separate
`HoodieTableMetadata` with `reuse=true` and returns without closing it. In Hudi
1.0.2 that mode retains base/log readers (and a nested metadata filesystem
view) until `HoodieBackedTableMetadata.close()`. The `fsView` closed below owns
a different metadata object, so an unpruned metadata-enabled scan still leaks
readers on every plan. Please make `listAllPartitionPaths` own that object with
try-with-resources (the returned list is already materialized) and cover the
close path in a test.
--
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]
