sunchao commented on code in PR #6054:
URL: https://github.com/apache/datafusion-comet/pull/6054#discussion_r4054628544
##########
spark/src/main/scala/org/apache/spark/Plugins.scala:
##########
@@ -176,6 +149,38 @@ object CometDriverPlugin extends Logging {
}
}
+ // Comet's native allocations are made by the Rust global allocator and live
in the native heap.
+ // The share that operators reserve is charged against a memory pool, but
everything else --
+ // expression kernels and Arrow array builders, decompression buffers,
Parquet reader structures,
+ // object store buffers, the tokio runtime, allocator overhead -- is covered
by no budget at all,
+ // and neither is Comet's JVM-side Arrow allocator. The only slack the
executor container has for
+ // that is spark.executor.memoryOverhead, which the JVM's own non-heap usage
already draws on.
+ //
+ // Comet used to add spark.comet.memoryOverhead to it here, but a driver
plugin cannot: on Spark
+ // 3.4, 3.5 and 4.0, SparkContext builds the default ResourceProfile before
it creates the plugin
+ // container, and the cluster managers size executors from that profile
rather than re-reading
+ // the conf, so the new value never reached the container. Say so while the
application is still
+ // starting up instead, because this has to be set before the SparkContext
is created.
+ private[apache] def warnIfExecutorMemoryOverheadUnset(conf: SparkConf): Unit
= {
+ val cometEnabled = getBooleanConf(conf, CometConf.COMET_ENABLED)
+ val cometExecEnabled = getBooleanConf(conf, CometConf.COMET_EXEC_ENABLED)
+ val cometShuffleEnabled = getBooleanConf(conf,
CometConf.COMET_SHUFFLE_ENABLED)
+ val cometActive = cometEnabled && (cometExecEnabled || cometShuffleEnabled)
+
+ if (cometActive && !conf.contains(EXECUTOR_MEMORY_OVERHEAD.key)) {
Review Comment:
### Correctness
[P2] Cover the managed-AM YARN client migration when overhead is already set
For explicitly enabled on-heap Comet on Spark 3.5/4.0, ordinary YARN client
mode (`spark.yarn.unmanagedAM.enabled=false`, the default) still used the
removed allowance. After plugin initialization,
`YarnClientSchedulerBackend.start` passes the updated `sc.conf` to `Client`,
which serializes it for a separate ApplicationMaster. `YarnAllocator` builds
and caches its own default resource profile from that configuration, so the
driver's earlier cached profile does not make this path inert.
For example, a 1 GiB heap with explicit 2 GiB Spark overhead and the default
1 GiB Comet pool previously requested 4 GiB. This change requests 3 GiB before
YARN rounding, while the native pool stays at 1 GiB. The [removed
code](https://github.com/apache/datafusion-comet/blob/f0787bcb4c2de95d5a9bddf9e009585aebe38414/spark/src/main/scala/org/apache/spark/Plugins.scala#L77-L91)
added the allowance even to explicit overhead values, but this condition
suppresses the migration warning for those applications.
Please cover this affected case in the warning and migration guidance,
correct the blanket claim that 3.5/4.0 had no behavior change, and add a
focused managed-AM client lifecycle check. Keeping the same request in this
example requires changing the existing explicit overhead from 2 GiB to 3 GiB.
Removing the automatic mutation can remain intentional. This is a
source-derived compatibility effect in the testing/on-heap mode, not a measured
OOM or a change to production off-heap sizing.
--
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]
