Skip to content

[SPARK-32293] Fix inconsistency between Spark memory configs and JVM option #29090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
attilapiros wants to merge 5 commits into from
Closed

[SPARK-32293] Fix inconsistency between Spark memory configs and JVM option #29090

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfbce91
Initial upload
attilapiros Jul 13, 2020
2ddbe0c
Extract common code into an util method
attilapiros Jul 14, 2020
cc495c1
use byteStringAsMb for driverMemory
attilapiros Jul 15, 2020
76a297d
add warning message when suffix is missing
attilapiros Aug 3, 2020
8dfe643
fix EXECUTOR_MEMORY check in UnifiedMemoryManager
attilapiros Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ private[deploy] class SparkSubmitArguments(args: Seq[String], env: Map[String, S
error("Driver memory must be a positive number")
}
if (executorMemory != null
&& Try(JavaUtils.byteStringAsBytes(executorMemory)).getOrElse(-1L) <= 0) {
&& Try(JavaUtils.byteStringAsMb(executorMemory)).getOrElse(-1L) <= 0) {

@dongjoon-hyun dongjoon-hyun Jul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this change executorMemory, do we need to update line 248 for driverMemory together maybe?

@dongjoon-hyun dongjoon-hyun Jul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, do we need to change this line? This seems to ensure that the value is non-negative. Just for my understanding, could you give me some example which gives different result before and after this PR?

@attilapiros attilapiros Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, there is no difference regarding the behaviour but reading the code and seeing byteStringAsBytes called with these configs gives the false impression they are in bytes. I think it is worth to change them to byteStringAsMb.

@mridulm mridulm Nov 3, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, the only difference would be if user set the memory to < 1 mb.
This is ridiculous enough to ignore as a valid usecase :-)

error("Executor memory must be a positive number")
}
if (executorCores != null && Try(executorCores.toInt).getOrElse(-1) <= 0) {
Expand Down
5 changes: 3 additions & 2 deletions docs/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ of the most common options to set are:
<td>
Amount of memory to use for the driver process, i.e. where SparkContext is initialized, in the
same format as JVM memory strings with a size unit suffix ("k", "m", "g" or "t")
(e.g. <code>512m</code>, <code>2g</code>).
(e.g. <code>512m</code>, <code>2g</code>) using "m" as the default unit.
<br />
<em>Note:</em> In client mode, this config must not be set through the <code>SparkConf</code>
directly in your application, because the driver JVM has already started at that point.
Expand Down Expand Up @@ -249,7 +249,8 @@ of the most common options to set are:
<td>1g</td>
<td>
Amount of memory to use per executor process, in the same format as JVM memory strings with
a size unit suffix ("k", "m", "g" or "t") (e.g. <code>512m</code>, <code>2g</code>).
a size unit suffix ("k", "m", "g" or "t") (e.g. <code>512m</code>, <code>2g</code>) using

@tgravescs tgravescs Jul 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we are a bit inconsistent across the documentation as wel (pyspark.memory, memoryOverhead)l. other memory settings just say MiB unless otherwise specified but don't mention the suffix options. I wonder if we make them all consistent. Note one of the yarn configs says: Use lower-case suffixes, e.g. k, m, g, t, and p, for kibi-, mebi-, gibi-, tebi-, and pebibytes, respectively. but again doesn't say m is the default.

"m" as the default unit.
</td>
<td>0.7.0</td>
</tr>
Expand Down
6 changes: 5 additions & 1 deletion docs/monitoring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,11 @@ The history server can be configured as follows:
<tr><th style="width:21%">Environment Variable</th><th>Meaning</th></tr>
<tr>
<td><code>SPARK_DAEMON_MEMORY</code></td>
<td>Memory to allocate to the history server (default: 1g).</td>
<td>
Memory to allocate to the history server (default: 1g). This can be configured in the same
format as JVM memory strings with a size unit suffix ("k", "m", "g" or "t") using "m" as
the default unit.
</td>
</tr>
<tr>
<td><code>SPARK_DAEMON_JAVA_OPTS</code></td>
Expand Down
5 changes: 4 additions & 1 deletion docs/spark-standalone.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ You can optionally configure the cluster further by setting environment variable
</tr>
<tr>
<td><code>SPARK_DAEMON_MEMORY</code></td>
<td>Memory to allocate to the Spark master and worker daemons themselves (default: 1g).</td>
<td>
Memory to allocate to the Spark master and worker daemons themselves (default: 1g). This can be configured in the same
format as JVM memory strings with a size unit suffix ("k", "m", "g" or "t") using "m" as the default unit.
</td>
</tr>
<tr>
<td><code>SPARK_DAEMON_JAVA_OPTS</code></td>
Expand Down
11 changes: 11 additions & 0 deletions launcher/src/main/java/org/apache/spark/launcher/CommandBuilderUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,4 +328,15 @@ static String findJarsDir(String sparkHome, String scalaVersion, boolean failIfN
return libdir.getAbsolutePath();
}

/**
* Add "m" as the default suffix unit when no explicit unit is given.
*/
static String addDefaultMSuffixIfNeeded(String memoryString) {
if (memoryString.chars().allMatch(Character::isDigit)) {
return memoryString + "m";
} else {
return memoryString;
}
}

}
2 changes: 1 addition & 1 deletion launcher/src/main/java/org/apache/spark/launcher/SparkClassCommandBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public List<String> buildCommand(Map<String, String> env)
}

String mem = firstNonEmpty(memKey != null ? System.getenv(memKey) : null, DEFAULT_MEM);
cmd.add("-Xmx" + mem);
cmd.add("-Xmx" + addDefaultMSuffixIfNeeded(mem));

@tgravescs tgravescs Jul 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update the standalone docs as well for --memory (SPARK_WORKER_MEMORY and SPARK_DAEMON_MEMORY) to say default to m and ideally make consistent with above docs.

@tgravescs tgravescs Jul 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we should test those as well if you haven't already

@attilapiros attilapiros Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I really focused on XMX and XMS settings but now I see there another error at

spark/core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala

Line 225 in cc495c1

val executorMemory = conf.getSizeAsBytes(config.EXECUTOR_MEMORY.key)

cmd.add(className);
cmd.addAll(classArgs);
return cmd;
Expand Down
2 changes: 1 addition & 1 deletion launcher/src/main/java/org/apache/spark/launcher/SparkSubmitCommandBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ private List<String> buildSparkSubmitCommand(Map<String, String> env)
isThriftServer(mainClass) ? System.getenv("SPARK_DAEMON_MEMORY") : null;
String memory = firstNonEmpty(tsMemory, config.get(SparkLauncher.DRIVER_MEMORY),
System.getenv("SPARK_DRIVER_MEMORY"), System.getenv("SPARK_MEM"), DEFAULT_MEM);
cmd.add("-Xmx" + memory);
cmd.add("-Xmx" + addDefaultMSuffixIfNeeded(memory));
addOptionString(cmd, driverDefaultJavaOptions);
addOptionString(cmd, driverExtraJavaOptions);
mergeEnvPathList(env, getLibPathEnvName(),
Expand Down
16 changes: 16 additions & 0 deletions launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,22 @@ public void testCliParser() throws Exception {
Collections.indexOfSubList(cmd, Arrays.asList(parser.CONF, "spark.randomOption=foo")) > 0);
}

@Test
public void testParserWithDefaultUnit() throws Exception {
List<String> sparkSubmitArgs = Arrays.asList(
parser.MASTER,
"local",
parser.DRIVER_MEMORY,
"4200",
parser.DRIVER_CLASS_PATH,
"/driverCp",
SparkLauncher.NO_RESOURCE);
Map<String, String> env = new HashMap<>();
List<String> cmd = buildCommand(sparkSubmitArgs, env);

assertTrue("Driver -Xmx should be configured in MB by default.", cmd.contains("-Xmx4200m"));
}

@Test
public void testShellCliParser() throws Exception {
List<String> sparkSubmitArgs = Arrays.asList(
Expand Down
9 changes: 7 additions & 2 deletions resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,16 @@ case "$1" in
;;
executor)
shift 1
MEMORY_WITH_UNIT=$SPARK_EXECUTOR_MEMORY
if [[ $MEMORY_WITH_UNIT =~ ^[0-9]+$ ]]
then
MEMORY_WITH_UNIT="${MEMORY_WITH_UNIT}m"
fi
CMD=(
${JAVA_HOME}/bin/java
"${SPARK_EXECUTOR_JAVA_OPTS[@]}"
-Xms$SPARK_EXECUTOR_MEMORY
-Xmx$SPARK_EXECUTOR_MEMORY
-Xms$MEMORY_WITH_UNIT
-Xmx$MEMORY_WITH_UNIT
-cp "$SPARK_CLASSPATH:$SPARK_DIST_CLASSPATH"
org.apache.spark.executor.CoarseGrainedExecutorBackend
--driver-url $SPARK_DRIVER_URL
Expand Down