github-actions[bot] commented on code in PR #66913:
URL: https://github.com/apache/doris/pull/66913#discussion_r3996745754
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -1709,7 +1723,109 @@ public void doStartSplit() throws UserException {
}
}
}
- }, Env.getCurrentEnv().getExtMetaCacheMgr().getScheduleExecutor());
+ });
+ splitAssignment.addCloseable(planningTask);
+ planningTask.submit();
+ }
+
+ /** Owns the planner generation until the worker actually exits, including
cancellation races. */
+ @VisibleForTesting
+ static final class AsyncPlanningTask implements Runnable, Closeable {
+ private final ExecutorService executor;
+ private final SplitAssignment splitAssignment;
+ private final Closeable generationLease;
+ private final Runnable planning;
+ private boolean submitted;
+ private boolean started;
+ private boolean closeRequested;
+ private boolean finished;
+ private Thread runner;
+
+ AsyncPlanningTask(ExecutorService executor, SplitAssignment
splitAssignment,
+ Closeable generationLease, Runnable planning) {
+ this.executor = Objects.requireNonNull(executor, "executor is
null");
+ this.splitAssignment = Objects.requireNonNull(splitAssignment,
"splitAssignment is null");
+ this.generationLease = Objects.requireNonNull(generationLease,
"generationLease is null");
+ this.planning = Objects.requireNonNull(planning, "planning is
null");
+ }
+
+ void submit() {
+ try {
+ synchronized (this) {
+ if (finished) {
+ return;
+ }
+ submitted = true;
+ }
+ executor.execute(this);
Review Comment:
[P2] Make cancellation visible across the enqueue gap
After this task sets submitted=true but before this execute call,
SplitAssignment.stop() can run close(): remove(this) finds nothing because
enqueue has not happened yet, and finish() marks the task dead. This call then
still enqueues that dead Runnable. If the shared bounded schedule queue is
full, the new BlockedPolicy cannot notice the cancellation either because it
only polls Future.isCancelled() and AsyncPlanningTask is a plain Runnable, so
the cancelled submitter can wait the full 10 seconds. The mocked
submission-race test misses both the real queue and full-queue path. Please use
a cancellable queue-visible task/protocol and recheck removal after enqueue,
with a bounded-executor regression for this exact ordering.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -110,7 +118,14 @@ public IcebergExternalMetaCache(ExecutorService
refreshExecutor, ExternalMetaCac
this::loadTableCacheValue, defaultEntryCacheSpec(),
MetaCacheEntryInvalidation.forNameMapping(nameMapping ->
nameMapping))
.withSizeEstimator(this::prepareTableForCachePublication)
- .withReplacementListener(this::retireTableGeneration));
+ .withReplacementListener(this::retireTableGeneration)
+ .withUnpublishedValueRetirer(IcebergTableCacheValue::retire)
+ .withRemovalListener(value -> value, (key, value) -> {
Review Comment:
[P1] Do not queue retired table graphs outside the cache budget
MetaCacheEntry releases the weighted reservation before enqueueing this
removal token, and its queue is unbounded behind one process-wide cleanup
thread. Because this extractor returns the IcebergTableCacheValue itself, every
eviction keeps the frozen table/metadata payload, runtime context, FileIO
owner, and catalog-generation lease strongly reachable after their accounted
weight has dropped to zero; churn can therefore admit replacement generations
while accumulating unbounded retired memory. The generic queue test uses a
small Long token and misses this registration. Please detach/queue a compact
idempotent cleanup owner, or keep queued bytes charged to a bounded budget, and
cover the real table-entry churn path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java:
##########
@@ -73,8 +86,39 @@ public class HMSExternalCatalog extends ExternalCatalog {
//for "type" = "hms" , but is iceberg table.
private IcebergMetadataOps icebergMetadataOps;
+ private IcebergCatalogResourceTracker icebergResourceTracker = new
IcebergCatalogResourceTracker();
private volatile AbstractHiveProperties hmsProperties;
+ private AtomicLong runtimeGeneration = new AtomicLong();
+
+ public synchronized long getRuntimeGeneration() {
Review Comment:
[P2] Keep per-split generation reads off the catalog monitor
Hudi now reaches this getter once for every FileSlice in
generateHudiSplit(), and again while serializing every split. Synchronizing the
AtomicLong read serializes all concurrent scans for the catalog and, during
ALTER/reset, makes every check wait behind synchronized onClose(), which can
spend up to 120 seconds shutting down executors and also closes metadata
clients under this monitor. The counter is incremented before runtime mutation,
so the read itself can be lock-free. Please remove synchronized here (or
consolidate the checks at the existing task/publication boundaries) so
high-cardinality scans do not bottleneck or stall behind catalog teardown.
--
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]
