You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/19 19:45:30 UTC
[PR] fix: remove the ineffective spark.executor.memoryOverhead adjustment from the driver plugin [datafusion-comet]
andygrove opened a new pull request, #6054:
URL: https://github.com/apache/datafusion-comet/pull/6054
## Which issue does this PR close?
Related to #6050. Replaces #6053, which went the other way (extending the adjustment to off-heap mode) before I checked whether the adjustment reaches the container at all.
## Rationale for this change
`CometDriverPlugin` adds `spark.comet.memoryOverhead` to `spark.executor.memoryOverhead` during `init`. On three of the five supported Spark versions that has no effect on anything.
`ResourceProfileManager`'s constructor eagerly materializes the default `ResourceProfile` (`private val defaultProfile = ResourceProfile.getOrCreateDefaultProfile(sparkConf)`), and `getDefaultExecutorResources` snapshots `conf.get(EXECUTOR_MEMORY_OVERHEAD)` into it. The cluster managers size executors from that profile rather than re-reading the conf: `BasicExecutorFeatureStep` builds the pod from `ResourceProfile.getResourcesForClusterManager(...)`, whose `memoryOverheadMiB` comes from `getDefaultProfileExecutorResources(conf)`, which returns the cached profile. YARN's allocator goes through the same call.
So whether the plugin can still influence it comes down to the ordering in `SparkContext`:
| Spark | `ResourceProfileManager` | `PluginContainer` | Mutation reaches the container? |
| ----- | ------------------------ | ----------------- | ------------------------------- |
| 3.4.3 | line 455 | line 565 | no |
| 3.5.8 | line 469 | line 574 | no |
| 4.0.0 | line 486 | line 588 | no |
| 4.1.3 | line 595 | line 594 | yes |
| 4.2.0 | line 595 | line 594 | yes |
On 3.4/3.5/4.0 the profile is already built and cached by the time any plugin's `init` runs, so the mutation updates `sc.conf` and nothing else. Nothing in Comet or Spark reads `spark.executor.memoryOverhead` at runtime, so on those versions it is inert. `CometPluginsSuite` never caught it because it asserts on `sc.conf`, which is updated either way, in local mode where nothing sizes a container.
That leaves a config knob that silently works on two versions and silently does nothing on three, plus a reassuring `logInfo` that reports an override that may not have happened. Deleting it is more honest than keeping it, and the thing that actually works on every version — the user setting `spark.executor.memoryOverhead` before the context starts — is now documented in #6051.
## What changes are included in this PR?
- `CometDriverPlugin.init` no longer touches `spark.executor.memoryOverhead`.
- In its place, `warnIfExecutorMemoryOverheadUnset` warns at startup when Comet is executing something and `spark.executor.memoryOverhead` was not set, explaining that Comet allocates outside the JVM heap and pointing at the tuning guide. This follows the existing `warnIfKryoRegistratorMissing` in the same file, which handles the same shape of problem: a setting that must be in place before the `SparkContext` is created.
- `CometSparkSessionExtensions.shouldOverrideMemoryConf` and its `getBooleanConf` helper are removed; they had no other callers.
- Both `ShimCometDriverPlugin` files are deleted. They existed only to paper over `EXECUTOR_MIN_MEMORY_OVERHEAD` not being present before Spark 4.0, which only the removed calculation needed.
- `CometConf.TUNING_GUIDE` becomes public so the warning can reference it, matching `COMPAT_GUIDE`.
- The stale comment on `getCometMemoryOverheadInMiB`'s off-heap early return is corrected: that `0` is about pool sizing (off-heap sizes the pool from `spark.memory.offHeap.size`), not about container overhead.
`spark.comet.memoryOverhead` itself is unchanged and still sizes the native memory pool in on-heap mode, which is its remaining job.
## Behavior change
On Spark 4.1 and 4.2, where the adjustment did reach the container, executor containers shrink by `spark.comet.memoryOverhead` (1 GiB by default) unless the application sets `spark.executor.memoryOverhead` itself. On-heap users on those versions should set it explicitly; the new warning tells them to, and the tuning guide in #6051 gives a starting value. On 3.4, 3.5 and 4.0 there is no behavior change, because there was no behavior.
## How are these changes tested?
- `CometPluginsSuite` asserts an explicitly set overhead is left alone, and `CometPluginsDefaultSuite` asserts an unset one stays unset.
- `CometPluginsUnifiedModeSuite` (renamed from `CometPluginsUnifiedModeOverrideSuite`) keeps an off-heap case so a future change that reintroduces a mutation there is caught. It also drops a `spark.comet.memory.overhead.factor` setting that stopped existing in #2599 and a comment computing a value from it.
- New `CometPluginsMemoryOverheadWarningSuite` covers the warning firing, not firing when the value is set, and not firing when Comet is not executing anything. The first case proves the appender sees the message, so the two negative cases are not vacuous.
- `CometPluginsNonOverrideSuite` is removed — with no mutation left, it asserted the same thing as `CometPluginsSuite`.
- The two `shouldOverrideMemoryConf` assertions in `CometSparkSessionExtensionsSuite` are dropped along with the method.
- CI suite registration in `pr_build_linux.yml` and `pr_build_macos.yml` updated for the renamed and new suites; `dev/ci/check-suites.py` and `dev/ci/check-ci-config.py` pass.
21 tests pass locally across the plugin and extension suites. `test-compile` is clean against Spark 3.4/Scala 2.12, 4.0, and 4.1, which matters here because two shim files were deleted. Spotless and scalastyle are clean.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: remove the ineffective spark.executor.memoryOverhead adjustment from the driver plugin [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6054:
URL: https://github.com/apache/datafusion-comet/pull/6054#issuecomment-5750715905
Thanks for the review @sunchao
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: remove the ineffective spark.executor.memoryOverhead adjustment from the driver plugin [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #6054:
URL: https://github.com/apache/datafusion-comet/pull/6054
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: remove the ineffective spark.executor.memoryOverhead adjustment from the driver plugin [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: remove the ineffective spark.executor.memoryOverhead adjustment from the driver plugin [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6054:
URL: https://github.com/apache/datafusion-comet/pull/6054#discussion_r4057145491
##########
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:
on-heap mode is only enabled for running Spark SQL tests locally and in CI. On-heap mode should never be used for a YARN deployment.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org