[SPARK-58513][CORE] UnifiedMemoryManager wrongly reports INVALID_DRIVER_MEMORY on the executor side - #57716
[SPARK-58513][CORE] UnifiedMemoryManager wrongly reports INVALID_DRIVER_MEMORY on the executor side#57716AngersZhuuuu wants to merge 9 commits into
Conversation
…nfig in driver , check executor config in executor
|
Thank you @AngersZhuuuu! |
|
This fix have a problem that may cause gluten failed to init, since we change teh method's parameter. So there have two way to fix this:
Which one do you think is better? @uros-b |
shrirangmhalgi
left a comment
There was a problem hiding this comment.
Thankyou @AngersZhuuuu for working on this. LGTM.
What I Verified
SparkContext.isDriver(executorId)isDRIVER_IDENTIFIER == executorId(string constant comparison, no SparkContext instance needed)apply(conf, numCores)2-arg overload preserved -- delegates toisDriver = Nonewhich runs both checks, identical to current behaviorisDriver = Some(true)→ driver check only;Some(false)→ executor check only- HashedRelation.scala uses
new UnifiedMemoryManager(...)directly (bypassesapply()/getMaxMemory()) -- unaffected by this change
Re: Gluten compatibility question
The existing apply(conf, numCores) signature is preserved with identical semantics - it delegates to isDriver = None which runs both checks, exactly as before. External callers (Gluten, etc.) will continue to work without changes. Neither Option 1 nor Option 2 is needed -- the current code already handles it.
nit: PR title has "Memeory". Could you please change it to "Memory".
| val minSystemMemory = (reservedMemory * 1.5).ceil.toLong | ||
| if (systemMemory < minSystemMemory) { | ||
| val checkDriverMemory = isDriver.isEmpty || isDriver.contains(true) | ||
| val checkExecutorMemory = isDriver.isEmpty || isDriver.contains(false) |
There was a problem hiding this comment.
This reverts the SPARK-12759 fail-fast: the driver no longer validates spark.executor.memory at SparkContext startup, so an undersized value now launches executors that all fail until the cluster manager gives up. The executor-memory check is cheap and correct on both roles; only the INVALID_DRIVER_MEMORY check needs gating:
if (isDriver.getOrElse(true) && systemMemory < minSystemMemory) { ... }
// SPARK-12759: keep unconditional
if (conf.contains(config.EXECUTOR_MEMORY)) { ... }There was a problem hiding this comment.
How about current?
There was a problem hiding this comment.
Overall this looks good to me. The wiring is consistent with the existing SparkContext.isDriver(executorId) usage already in SparkEnv (e.g. the streaming-shuffle-output-tracker selection), local mode keeps isDriver=true, and the behavior change is purely which exception/message is thrown on the executor side — no pass/fail condition changes, so no valid configuration newly fails or passes. The 2-arg apply defaulting to isDriver=true keeps every existing/test caller intact.
A few nits below. Also worth noting for the stale description: the executor still runs both checks — the runtime-heap check (the point of this PR) and the SPARK-12759 spark.executor.memory config check — same on the driver. So "validates spark.executor.memory only" isn't quite right.
| def apply( | ||
| conf: SparkConf, | ||
| numCores: Int, | ||
| isDriver: Boolean): UnifiedMemoryManager = { |
There was a problem hiding this comment.
This declaration fits the 100-char limit on one line — def apply(conf: SparkConf, numCores: Int, isDriver: Boolean): UnifiedMemoryManager = { is ~88 chars. Suggest collapsing to a single line for consistency with the other one-liners in this companion object.
| throw new SparkIllegalArgumentException( | ||
| errorClass = "INVALID_EXECUTOR_MEMORY", | ||
| messageParameters = Map( | ||
| "executorMemory" -> systemMemory.toString, |
There was a problem hiding this comment.
INVALID_EXECUTOR_MEMORY now means two different "memory" values depending on which branch fires. The existing SPARK-12759 check below (~line 492) reports the configured spark.executor.memory through this same error class/parameter, whereas here the executor branch passes systemMemory (the observed JVM heap, Runtime.maxMemory) as executorMemory. The rendered message ("Executor memory 466092032 must be at least 471859200") therefore shows the runtime heap rather than the 500m the user actually configured — still a bit misleading, just differently than before.
Consider a dedicated error class framed as "System memory ... increase --executor-memory" (mirroring how INVALID_DRIVER_MEMORY says "System memory"), or surface both the observed and configured values.
| assert(exception.getMessage.contains("increase executor memory")) | ||
| } | ||
|
|
||
| test("SPARK-58513: driver validates executor memory") { |
There was a problem hiding this comment.
This test exercises pre-existing behavior, not the new code path. With systemMemory=1MB > minSystemMemory, the first check passes and this hits the pre-existing SPARK-12759 config check — effectively a duplicate of the existing "insufficient executor memory" test (line 243) but with explicit isDriver=true. The test that actually validates the new behavior is "SPARK-58513: executor validates executor heap" above.
Consider dropping this one, or repurposing it (e.g. assert the driver small-heap path emits "increase heap size", which is distinct from the executor message).
pan3793
left a comment
There was a problem hiding this comment.
LGTM. @uros-b @shrirangmhalgi, do you want to have another look? since things get changed after your approval
|
@AngersZhuuuu, the PySpark CI failure is unrelated, could you refresh the PR description to match? It still describes the previous revision. |
Done |
Fold INVALID_EXECUTOR_SYSTEM_MEMORY into INVALID_EXECUTOR_MEMORY as SYSTEM_MEMORY/CONFIG_MEMORY sub-conditions, and restructure INVALID_DRIVER_MEMORY with a single SYSTEM_MEMORY sub-condition. Update throw sites and tests accordingly. Assisted-by: Claude Fable 5
…ER_MEMORY on the executor side ### What changes were proposed in this pull request? Make `UnifiedMemoryManager` report role-appropriate errors when the JVM heap is too small: - `SparkEnv` passes the process role (`isDriver`) to `UnifiedMemoryManager`. - The system memory check throws `INVALID_DRIVER_MEMORY.SYSTEM_MEMORY` on the driver and the new `INVALID_EXECUTOR_MEMORY.SYSTEM_MEMORY` on executors, each pointing to its own memory option. - The SPARK-12759 fail-fast check of `spark.executor.memory` is unchanged and runs on both roles, now as `INVALID_EXECUTOR_MEMORY.CONFIG_MEMORY`. - The two error conditions are restructured with sub-conditions: `INVALID_DRIVER_MEMORY.SYSTEM_MEMORY`, `INVALID_EXECUTOR_MEMORY.{CONFIG_MEMORY, SYSTEM_MEMORY}`. ### Why are the changes needed? When `spark.executor.memory` is set to 500m, the executor may fail with: ``` Exception in thread "main" java.lang.IllegalArgumentException: System memory 466092032 must be at least 471859200. Please increase heap size using the --driver-memory option or spark.driver.memory in Spark configuration. ``` The message is misleading: the check runs in the executor, whose heap is controlled by `spark.executor.memory`, but it reports driver configuration guidance. ### Does this PR introduce _any_ user-facing change? Yes, error reporting only; no pass/fail behavior change. - An executor with insufficient heap now fails with `INVALID_EXECUTOR_MEMORY.SYSTEM_MEMORY`, pointing to `--executor-memory` / `spark.executor.memory`, instead of `INVALID_DRIVER_MEMORY`. - Existing conditions become sub-conditions: `INVALID_DRIVER_MEMORY` -> `INVALID_DRIVER_MEMORY.SYSTEM_MEMORY`, `INVALID_EXECUTOR_MEMORY` -> `INVALID_EXECUTOR_MEMORY.CONFIG_MEMORY`; messages gain an "Insufficient driver/executor memory:" prefix. ### How was this patch tested? Added and updated UTs in `UnifiedMemoryManagerSuite`; error definitions are validated by `SparkThrowableSuite`. ### Was this patch authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Claude Fable 5) Closes #57716 from AngersZhuuuu/SPARK-58513. Lead-authored-by: Angerszhuuuu <angers.zhu@gmail.com> Co-authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit f2815e0) Signed-off-by: Cheng Pan <chengpan@apache.org>
…ER_MEMORY on the executor side ### What changes were proposed in this pull request? Make `UnifiedMemoryManager` report role-appropriate errors when the JVM heap is too small: - `SparkEnv` passes the process role (`isDriver`) to `UnifiedMemoryManager`. - The system memory check throws `INVALID_DRIVER_MEMORY.SYSTEM_MEMORY` on the driver and the new `INVALID_EXECUTOR_MEMORY.SYSTEM_MEMORY` on executors, each pointing to its own memory option. - The SPARK-12759 fail-fast check of `spark.executor.memory` is unchanged and runs on both roles, now as `INVALID_EXECUTOR_MEMORY.CONFIG_MEMORY`. - The two error conditions are restructured with sub-conditions: `INVALID_DRIVER_MEMORY.SYSTEM_MEMORY`, `INVALID_EXECUTOR_MEMORY.{CONFIG_MEMORY, SYSTEM_MEMORY}`. ### Why are the changes needed? When `spark.executor.memory` is set to 500m, the executor may fail with: ``` Exception in thread "main" java.lang.IllegalArgumentException: System memory 466092032 must be at least 471859200. Please increase heap size using the --driver-memory option or spark.driver.memory in Spark configuration. ``` The message is misleading: the check runs in the executor, whose heap is controlled by `spark.executor.memory`, but it reports driver configuration guidance. ### Does this PR introduce _any_ user-facing change? Yes, error reporting only; no pass/fail behavior change. - An executor with insufficient heap now fails with `INVALID_EXECUTOR_MEMORY.SYSTEM_MEMORY`, pointing to `--executor-memory` / `spark.executor.memory`, instead of `INVALID_DRIVER_MEMORY`. - Existing conditions become sub-conditions: `INVALID_DRIVER_MEMORY` -> `INVALID_DRIVER_MEMORY.SYSTEM_MEMORY`, `INVALID_EXECUTOR_MEMORY` -> `INVALID_EXECUTOR_MEMORY.CONFIG_MEMORY`; messages gain an "Insufficient driver/executor memory:" prefix. ### How was this patch tested? Added and updated UTs in `UnifiedMemoryManagerSuite`; error definitions are validated by `SparkThrowableSuite`. ### Was this patch authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Claude Fable 5) Closes #57716 from AngersZhuuuu/SPARK-58513. Lead-authored-by: Angerszhuuuu <angers.zhu@gmail.com> Co-authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit f2815e0) Signed-off-by: Cheng Pan <chengpan@apache.org>
What changes were proposed in this pull request?
Make
UnifiedMemoryManagerreport role-appropriate errors when the JVM heap is too small:SparkEnvpasses the process role (isDriver) toUnifiedMemoryManager.INVALID_DRIVER_MEMORY.SYSTEM_MEMORYon the driver and the newINVALID_EXECUTOR_MEMORY.SYSTEM_MEMORYon executors, each pointing to its own memory option.spark.executor.memoryis unchanged and runs on both roles, now asINVALID_EXECUTOR_MEMORY.CONFIG_MEMORY.INVALID_DRIVER_MEMORY.SYSTEM_MEMORY,INVALID_EXECUTOR_MEMORY.{CONFIG_MEMORY, SYSTEM_MEMORY}.Why are the changes needed?
When
spark.executor.memoryis set to 500m, the executor may fail with:The message is misleading: the check runs in the executor, whose heap is controlled by
spark.executor.memory, but it reports driver configuration guidance.Does this PR introduce any user-facing change?
Yes, error reporting only; no pass/fail behavior change.
INVALID_EXECUTOR_MEMORY.SYSTEM_MEMORY, pointing to--executor-memory/spark.executor.memory, instead ofINVALID_DRIVER_MEMORY.INVALID_DRIVER_MEMORY->INVALID_DRIVER_MEMORY.SYSTEM_MEMORY,INVALID_EXECUTOR_MEMORY->INVALID_EXECUTOR_MEMORY.CONFIG_MEMORY; messages gain an "Insufficient driver/executor memory:" prefix.How was this patch tested?
Added and updated UTs in
UnifiedMemoryManagerSuite; error definitions are validated bySparkThrowableSuite.Was this patch authored or co-authored using generative AI tooling?
Yes.
Generated-by: Claude Code (Claude Fable 5)