[SPARK-53176][DEPLOY] Spark launcher should respect `--load-spark-defaults` #51905

pan3793 · 2025-08-07T16:22:36Z

What changes were proposed in this pull request?

SPARK-48392 introduces --load-spark-defaults, but does not apply correctly for the Spark launcher process, this mainly affects the driver when Spark runs in local/client mode.

let's say we have

$ cat > conf/spark-defaults.conf <<EOF
spark.driver.memory=4g
EOF
$ cat > conf/spark-local.conf <<EOF
spark.master=local[4]
EOF

$ bin/spark-shell --properties-file conf/spark-local.conf --load-spark-defaults
...
scala> spark.sql("SET spark.driver.memory").show()
+-------------------+-----+
|                key|value|
+-------------------+-----+
|spark.driver.memory|   4g|
+-------------------+-----+

even the spark conf reports that driver uses 4GiB heap memory, but if we check the Java process, the config actually does not take effect, the default 1GiB is used instead.

$ jinfo <spark-submit-pid>
...
VM Arguments:
jvm_args: -Dscala.usejavacp=true -Xmx1g ...

Why are the changes needed?

Bug fix.

Does this PR introduce any user-facing change?

Yes, bug fix.

How was this patch tested?

UT is modified to cover the change, plus manual tests for the above cases.

$ jinfo <spark-submit-pid>
...
VM Arguments:
jvm_args: -Dscala.usejavacp=true -Xmx4g ...

Was this patch authored or co-authored using generative AI tooling?

No.

…aults`

pan3793 · 2025-08-07T16:24:51Z

launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java

@@ -369,8 +372,13 @@ private void testCmdBuilder(boolean isDriver, File propertiesFile) throws Except
    String[] cp = findArgValue(cmd, "-cp").split(Pattern.quote(File.pathSeparator));
    if (isDriver) {
      assertTrue(contains("/driver", cp), "Driver classpath should contain provided entry.");
+      if (propertiesFile == null || loadSparkDefaults) {
+        assertTrue(contains("/driver-default", cp),
+          "Driver classpath should contain provided entry.");


this assertion fails without this patch.

pan3793 · 2025-08-07T17:27:37Z

cc @sunchao @viirya @cloud-fan

sunchao

LGTM, thanks!

launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java

viirya · 2025-08-08T05:42:52Z

launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java

+
+    for (Map.Entry<Object, Object> entry : props.entrySet()) {
+      if (!defaultsProps.containsKey(entry.getKey())) {
+        defaultsProps.put(entry.getKey(), entry.getValue());


Hmm? I think the logic should be that if user specified prop file contains any default entries, they must be overwritten. But here it looks like if the user specified entries are not in default prop file, they will be present? So it is a merger instead of overwritting? Is it correct?

Oops, you are right, will fix soon.

@viirya I fixed the behavior, also refactored UT to verify the priority of the configs:

--conf

then the user-specified properties file

then spark-defaults.conf if it exists

viirya · 2025-08-08T07:11:07Z

launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java

+        }
+      }
+      if (loadSparkDefaults) {
+        assertEquals("/driver", effectiveConfig.get(SparkLauncher.DRIVER_EXTRA_CLASSPATH));


Only the default props contain DRIVER_EXTRA_CLASSPATH?

Yes, I reuse the existing spark-defaults.conf
https://github.com/apache/spark/blob/master/launcher/src/test/resources/spark-defaults.conf

viirya · 2025-08-08T07:13:03Z

launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java

+  }
+
+  private void doTestGetEffectiveConfig(
+      File propertiesFile, boolean loadSparkDefaults, boolean confDriverMemory) throws Exception {


Does this test use loadSparkDefaults to conditionally check something? Seems no?

used at line 118

viirya · 2025-08-08T07:13:50Z

launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java

+        assertFalse(effectiveConfig.containsKey(SparkLauncher.DRIVER_EXTRA_CLASSPATH));
+      }
+    } else {
+      assertEquals("/driver", effectiveConfig.get(SparkLauncher.DRIVER_EXTRA_CLASSPATH));


Does this require loadSparkDefaults to be true?

it's only present at spark-defaults.conf

pan3793 · 2025-08-11T16:14:18Z

@viirya could you take another look?

viirya · 2025-08-11T16:22:16Z

I will take another look today. Thanks.

viirya · 2025-08-11T19:13:12Z

Thanks for waiting @pan3793. Looks good to me.

viirya · 2025-08-11T19:14:15Z

@sunchao Do you want to take another look on the change after you approved?

sunchao · 2025-08-11T21:29:03Z

Thanks @viirya . It looks good to me too!

…aults` ### What changes were proposed in this pull request? SPARK-48392 introduces `--load-spark-defaults`, but does not apply correctly for the Spark launcher process, this mainly affects the driver when Spark runs in local/client mode. let's say we have ``` $ cat > conf/spark-defaults.conf <<EOF spark.driver.memory=4g EOF $ cat > conf/spark-local.conf <<EOF spark.master=local[4] EOF ``` ``` $ bin/spark-shell --properties-file conf/spark-local.conf --load-spark-defaults ... scala> spark.sql("SET spark.driver.memory").show() +-------------------+-----+ | key|value| +-------------------+-----+ |spark.driver.memory| 4g| +-------------------+-----+ ``` even the spark conf reports that driver uses 4GiB heap memory, but if we check the Java process, the config actually does not take effect, the default 1GiB is used instead. ``` $ jinfo <spark-submit-pid> ... VM Arguments: jvm_args: -Dscala.usejavacp=true -Xmx1g ... ``` ### Why are the changes needed? Bug fix. ### Does this PR introduce _any_ user-facing change? Yes, bug fix. ### How was this patch tested? UT is modified to cover the change, plus manual tests for the above cases. ``` $ jinfo <spark-submit-pid> ... VM Arguments: jvm_args: -Dscala.usejavacp=true -Xmx4g ... ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51905 from pan3793/SPARK-53176. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit b82957c) Signed-off-by: Liang-Chi Hsieh <[email protected]>

viirya · 2025-08-11T22:16:00Z

Merged to master/4.0.

Thanks @pan3793 @sunchao

[SPARK-53176][DEPLOY] Spark launcher should respect `--load-spark-def…

0d17a4c

…aults`

pan3793 commented Aug 7, 2025

View reviewed changes

sunchao approved these changes Aug 8, 2025

View reviewed changes

launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java Outdated Show resolved Hide resolved

viirya reviewed Aug 8, 2025

View reviewed changes

pan3793 added 4 commits August 8, 2025 13:53

fix order

7a332ab

refactor UT

f9e6efa

nit

d7c31da

nit fix

2b431ec

viirya reviewed Aug 8, 2025

View reviewed changes

pan3793 requested a review from viirya August 9, 2025 09:52

viirya approved these changes Aug 11, 2025

View reviewed changes

viirya closed this in b82957c Aug 11, 2025

[SPARK-53176][DEPLOY] Spark launcher should respect --load-spark-defaults #51905

[SPARK-53176][DEPLOY] Spark launcher should respect --load-spark-defaults #51905

Uh oh!

Conversation

pan3793 commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Aug 7, 2025

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Aug 11, 2025

Uh oh!

viirya commented Aug 11, 2025

Uh oh!

viirya commented Aug 11, 2025

Uh oh!

viirya commented Aug 11, 2025

Uh oh!

sunchao commented Aug 11, 2025

Uh oh!

viirya commented Aug 11, 2025

Uh oh!

Uh oh!

[SPARK-53176][DEPLOY] Spark launcher should respect `--load-spark-defaults` #51905

[SPARK-53176][DEPLOY] Spark launcher should respect `--load-spark-defaults` #51905

pan3793 commented Aug 7, 2025 •

edited

Loading