andygrove opened a new pull request, #6053:
URL: https://github.com/apache/datafusion-comet/pull/6053
## Which issue does this PR close?
Related to #6050. Draft for discussion — this implements option 2/3 from
that issue (auto-adjust), and there is an open question below about whether it
can work at all on Spark 3.4, 3.5, and 4.0.
## Rationale for this change
`CometDriverPlugin` adds `spark.comet.memoryOverhead` to
`spark.executor.memoryOverhead` only in on-heap mode. In off-heap mode
`shouldOverrideMemoryConf` returns `false` and `getCometMemoryOverheadInMiB`
returns `0`, so the configuration we recommend to everyone gets no adjustment
at all.
As traced in #6050, no change ever decided this on the merits. #1379 made
off-heap mode add `getCometShuffleMemorySize` instead of the full overhead;
#1485 removed the test-only config that predicate depended on, which turned the
off-heap branch into dead code; #1525 deleted the dead branch and #1561 wrote
the resulting state up as intentional.
The premise in that comment — "we use unified memory management to share
off-heap memory with Spark so do not add overhead" — only covers the memory
operators actually reserve. Reservations are charged against
`spark.memory.offHeap.size`, which the cluster manager already folds into the
container size, so those have room. Everything Comet allocates without
reserving it does not: expression kernels and Arrow array builders,
decompression buffers and Parquet reader structures, object store request
buffers and the tokio runtime, Comet's JVM-side Arrow buffers, and allocator
overhead. Those come from the Rust global allocator and live in the native
heap, and `spark.executor.memoryOverhead` is the only slack in the container
for them.
## What changes are included in this PR?
- `shouldOverrideMemoryConf` no longer excludes off-heap mode, and
`getCometMemoryOverheadInMiB` no longer returns `0` there, so the plugin adds
`spark.comet.memoryOverhead` (default 1 GiB) to `spark.executor.memoryOverhead`
in both modes.
- `COMET_ONHEAP_MEMORY_OVERHEAD` renamed to `COMET_MEMORY_OVERHEAD` (the key
`spark.comet.memoryOverhead` is unchanged), moved from `CATEGORY_TESTING` to
`CATEGORY_TUNING`, and documented for both modes. It remains the native pool
size in on-heap mode; in off-heap mode the pool is still sized from
`spark.memory.offHeap.size`, so pool sizing is unchanged in both modes.
- The plugin's `else` branch now reports the reason it did not override
(Comet not executing anything) rather than the off-heap reason, which no longer
applies.
- Tests: `CometPluginsUnifiedModeOverrideSuite` now asserts the overhead
*is* increased in off-heap mode, with a new
`CometPluginsUnifiedModeNonOverrideSuite` covering off-heap with Comet exec and
shuffle both disabled. `CometSparkSessionExtensionsSuite`'s off-heap case is
flipped and gains a case for the disabled path. Three stale comments
referencing a 384 MB default are corrected to 1024 MB.
## Open question: this may be a no-op on Spark 3.4, 3.5, and 4.0
I could not find a way for the driver plugin's mutation to reach the
executor container on three of the five supported Spark versions, and if that
holds it applies to the existing on-heap bump too, not just to this change.
`ResourceProfileManager`'s constructor materializes the default
`ResourceProfile` eagerly (`private val defaultProfile =
ResourceProfile.getOrCreateDefaultProfile(sparkConf)`), and
`getDefaultExecutorResources` snapshots `conf.get(EXECUTOR_MEMORY_OVERHEAD)`
into it. The cluster managers size executors from that profile, not from a
fresh conf read: `BasicExecutorFeatureStep` builds the pod from
`ResourceProfile.getResourcesForClusterManager(...)`, whose `memoryOverheadMiB`
comes from `getDefaultProfileExecutorResources(conf)` → the cached profile.
So the ordering in `SparkContext` decides whether the plugin can still
influence it:
| Spark | `ResourceProfileManager` | `PluginContainer` | Plugin 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
|
`CometPluginsSuite` does not catch this because it asserts on `sc.conf`,
which is updated either way, and it runs in local mode where nothing sizes a
container.
If that is right, then on 3.4/3.5/4.0 the only thing that works is for the
user to set `spark.executor.memoryOverhead` before the context starts — which
is what #6051 now documents — and the warn-only option from #6050 becomes the
more useful change rather than merely the more conservative one. Worth a second
pair of eyes on the analysis before we decide; I have not reproduced it on a
real cluster.
## Other things worth deciding
- **Reusing `spark.comet.memoryOverhead` for both modes.** It means one
knob, but the knob means two different things: in on-heap mode it sizes the
native pool (and the matching container headroom), while in off-heap mode it is
purely container headroom for unreserved allocations. A separate off-heap
config would be clearer at the cost of another setting.
- **The default is a behavior change.** Every off-heap Comet user's
container grows by 1 GiB on Kubernetes and YARN, which changes bin-packing,
quotas, and capacity planning for clusters that are fine today. If we want that
to be opt-in, the cleanest form is probably a default of `0` in off-heap mode.
- **Interaction with #6051.** That PR recommends setting
`spark.executor.memoryOverhead=2g`. If this lands, a user following that advice
gets 2 GiB + 1 GiB. The tuning guide should be updated to say so once both are
settled.
## How are these changes tested?
`CometPluginsSuite`, `CometPluginsDefaultSuite`,
`CometPluginsNonOverrideSuite`, `CometPluginsUnifiedModeOverrideSuite`,
`CometPluginsUnifiedModeNonOverrideSuite`, and
`CometSparkSessionExtensionsSuite` pass locally against the default profile.
The version-dependent gap described above is not covered by any test, and I
do not think it can be in local mode.
--
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]
