peterxcli commented on PR #5493: URL: https://github.com/apache/datafusion-comet/pull/5493#issuecomment-5454994425
Addressed the [two follow-up witnesses](https://github.com/apache/datafusion-comet/pull/5493#pullrequestreview-5052924155) in d9bf677f0 — and I think together they settle the design question, so I changed approach rather than patching the detector again. Your cache-lock chain shows any frame- or state-based check can be evaded transitively (the blocked holder's stack shows the lock, not the Spark-memory dependency behind it), and your second witness shows the same check can misfire on a holder that an independently progressing task would have released. Holders' dependencies live outside this allocator's view, so no local detector classifies them reliably in either direction. The frame detector is removed. In its place the wait now has an overall bound, `spark.comet.shuffle.jvm.memoryWaitTimeout` (internal, default 5 minutes): on timeout the waiter throws the managed `SparkOutOfMemoryError`, exactly as BASE did, so its task releases memory, the resource on the other side of any cycle can progress, and Spark's task retry can recover. The two precise fail-fast checks (self-retained, all-waiters) are unchanged, and the interval re-logging remains. The new-waiter `notifyAll` concern is moot with the detector gone — a waiter now only gives up at its own deadline, never because another waiter's arrival changed an instantaneous classification. On base-relative behavior, stated plainly: with cross-task force-spill removed (the core of this PR), some schedules BASE completed by force-spilling another task's buffers can only ever complete here if the holder frees within the bound; the generous default covers every healthy-holder timing raised in this review (1.5 s, 65 s, independent finite releases), undetectable cycles now unwind within the bound instead of hanging the executor, and operators can tune the tradeoff. The cross-pool regression test now exercises the timeout (holder genuinely parked in `ExecutionMemoryPool`, waiter unwinds at a 1 s configured bound, pool drains fully once Spark memory frees). -- 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]
