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: [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]
