sunchao commented on PR #5668: URL: https://github.com/apache/datafusion-comet/pull/5668#issuecomment-5542164836
Re-reviewed `f54d00bd` against `4219fc79`. **The previous three findings are fixed. I found one new P2 issue.** **[P2] Completion callbacks can starve behind blocked dependency preparation.** In [CometCelebornShuffleMaterialization.scala:115–125](https://github.com/apache/datafusion-comet/blob/f54d00bd0736f6a7fb13c800eee1e46711258c6b/spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/CometCelebornShuffleMaterialization.scala#L115-L125), dependency preparation blocks on upstream materialization using `ExecutionContext.global`, while upstream completion callbacks require that same pool. Enough waiting dependencies exhaust its capacity: Spark finishes the upstream job, but Comet cannot publish completion, leaving downstream work stuck. I independently reproduced this using current Comet classes, native upstream execution, two visible driver CPUs, and Scala’s default pool settings: | Pending downstream dependencies | Global threads | Upstream materialization | Downstream jobs admitted | |---|---:|---|---:| | 240 | 241 | Completed | 240 | | 270 | 257 | Stuck after Spark job succeeded | 0 | Prefer asynchronous composition of upstream completion, or ensure completion callbacks have execution capacity independent of blocked preparation workers. Increasing the thread limit only moves the threshold. Reproduction and logs (local files). For the requested dimensions: - **Performance:** The issue above causes substantial driver-thread growth and eventually stalls progress. I found no other material overhead in the native success path. - **Design:** The sibling RDD correctly isolates replacement scheduling, and deferred waiting restores independent branch startup. The shared executor still needs attention. - **Abstraction and complexity:** Per-destination output metrics and the materialization state machine have clear responsibilities. I found no unnecessary abstraction worth removing. Validation: **24/24 focused Spark tests passed locally**. Current CI has **65 successful checks and 9 skipped**; its [Spark 4.1 shuffle suite](https://github.com/apache/datafusion-comet/actions/runs/33847325617/job/100949432090) passed **463/463 tests**. Local tests reused the checksum-verified native artifact because native sources were unchanged. The starvation reproduction uses local Spark and a fake Celeborn client; it does not establish distributed behavior or whole-query throughput. -- 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]
