pan3793 commented on code in PR #57716:
URL: https://github.com/apache/spark/pull/57716#discussion_r3734963359
##########
core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala:
##########
@@ -447,7 +447,14 @@ object UnifiedMemoryManager extends Logging {
}
def apply(conf: SparkConf, numCores: Int): UnifiedMemoryManager = {
- val maxMemory = getMaxMemory(conf)
+ apply(conf, numCores, isDriver = true)
+ }
+
+ def apply(
+ conf: SparkConf,
+ numCores: Int,
+ isDriver: Boolean): UnifiedMemoryManager = {
Review 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.
##########
core/src/test/scala/org/apache/spark/memory/UnifiedMemoryManagerSuite.scala:
##########
@@ -259,6 +259,38 @@ class UnifiedMemoryManagerSuite extends MemoryManagerSuite
with PrivateMethodTes
assert(exception.getMessage.contains("increase executor memory"))
}
+ test("SPARK-58513: executor validates executor heap") {
+ val systemMemory = 400L * 1024
+ val reservedMemory = 300L * 1024
+ val memoryFraction = 0.8
+ val conf = new SparkConf()
+ .set(MEMORY_FRACTION, memoryFraction)
+ .set(TEST_MEMORY, systemMemory)
+ .set(TEST_RESERVED_MEMORY, reservedMemory)
+ .set(EXECUTOR_MEMORY.key, (500L * 1024).toString)
+
+ val exception = intercept[IllegalArgumentException] {
+ UnifiedMemoryManager(conf, numCores = 1, isDriver = false)
+ }
+ assert(exception.getMessage.contains("increase executor memory"))
+ }
+
+ test("SPARK-58513: driver validates executor memory") {
Review 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).
##########
core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala:
##########
@@ -459,18 +466,27 @@ object UnifiedMemoryManager extends Logging {
/**
* Return the total amount of memory shared between execution and storage,
in bytes.
*/
- private def getMaxMemory(conf: SparkConf): Long = {
+ private def getMaxMemory(conf: SparkConf, isDriver: Boolean): Long = {
val systemMemory = conf.get(TEST_MEMORY)
val reservedMemory = conf.getLong(TEST_RESERVED_MEMORY.key,
if (conf.contains(IS_TESTING)) 0 else RESERVED_SYSTEM_MEMORY_BYTES)
val minSystemMemory = (reservedMemory * 1.5).ceil.toLong
if (systemMemory < minSystemMemory) {
- throw new SparkIllegalArgumentException(
- errorClass = "INVALID_DRIVER_MEMORY",
- messageParameters = Map(
- "systemMemory" -> systemMemory.toString,
- "minSystemMemory" -> minSystemMemory.toString,
- "config" -> config.DRIVER_MEMORY.key))
+ if (isDriver) {
+ throw new SparkIllegalArgumentException(
+ errorClass = "INVALID_DRIVER_MEMORY",
+ messageParameters = Map(
+ "systemMemory" -> systemMemory.toString,
+ "minSystemMemory" -> minSystemMemory.toString,
+ "config" -> config.DRIVER_MEMORY.key))
+ } else {
+ throw new SparkIllegalArgumentException(
+ errorClass = "INVALID_EXECUTOR_MEMORY",
+ messageParameters = Map(
+ "executorMemory" -> systemMemory.toString,
Review 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.
--
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]
