[SPARK-23472][CORE] Add defaultJavaOptions for driver and executor. #24804

gaborgsomogyi · 2019-06-05T08:25:07Z

What changes were proposed in this pull request?

This PR adds two new config properties: spark.driver.defaultJavaOptions and spark.executor.defaultJavaOptions. These are intended to be set by administrators in a file of defaults for options like JVM garbage collection algorithm. Users will still set extraJavaOptions properties, and both sets of JVM options will be added to start a JVM (default options are prepended to extra options).

How was this patch tested?

Existing + additional unit tests.

cd docs/
SKIP_API=1 jekyll build

Manual webpage check.

gaborgsomogyi · 2019-06-05T10:23:36Z

cc @vanzin @squito @rdblue since you were involved in the previous PR.

I've considered many options but this looks the best from my perspective.
I personally don't like the fact that it's not possible to concatenate options (just like Imran) but I think introducing it would produce more problems than solutions. Setting it in one (now two) places requires attention from user side (more coding to merge options properly) but then I think it's more clear what exactly will be set.

SparkQA · 2019-06-05T10:52:31Z

Test build #106196 has finished for PR 24804 at commit 7402291.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2019-06-05T16:17:09Z

You'll need to add similar logic to SparkSubmitCommandBuilder.java so that the client mode driver also picks up this new setting. Bonus points for making it generic so that it can be easily applied to future default opts we add that affect the driver.

gaborgsomogyi · 2019-06-06T18:37:00Z

Somehow I had the feeling this code part works with SparkConf and default fetched with extra (which is wrong). Not sure how this can be made more generic.

SparkQA · 2019-06-06T20:44:42Z

Test build #106250 has finished for PR 24804 at commit 9f41dca.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-06-06T21:13:36Z

Test build #106251 has finished for PR 24804 at commit ea50aef.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin

Looks ok.

core/src/main/scala/org/apache/spark/internal/config/package.scala

core/src/test/scala/org/apache/spark/internal/config/ConfigEntrySuite.scala

vanzin · 2019-06-24T23:45:00Z

BTW it would be good to double check the places where cluster-mode drivers are started and make sure they also pick up the new config.

e.g. StandaloneSubmitRequestServlet seems to create a driver command line and it does this:

val driverExtraJavaOptions = sparkProperties.get(config.DRIVER_JAVA_OPTIONS.key)

Which I think would completely ignore the stuff you're adding here...

* Removed unused config entries * Fallback test added * Fixed Mesos and Rest server property handling

SparkQA · 2019-06-26T17:00:31Z

Test build #106934 has finished for PR 24804 at commit aeb7cce.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-06-26T19:27:16Z

Test build #106941 has finished for PR 24804 at commit a7d7327.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

squito

looks ok, though I haven't looked at whether this is getting used everywhere it needs to be yet (marcelo's earlier comment)

docs/configuration.md

core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala

SparkQA · 2019-06-28T09:58:32Z

Test build #106993 has finished for PR 24804 at commit a5224fd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi · 2019-06-28T10:20:42Z

retest this please

SparkQA · 2019-06-28T12:47:49Z

Test build #106998 has finished for PR 24804 at commit a5224fd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-06-28T19:22:03Z

Test build #107017 has finished for PR 24804 at commit f08c2c6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin

Small doc nit; there's also MesosCoarseGrainedSchedulerBackend.scala which seems easy to fix to add support for this. There might be another spot in standalone that may need a change, but I'm having a hard time following that code... if someone cares about it they can figure it out, I guess.

docs/configuration.md

gaborgsomogyi · 2019-07-01T11:47:24Z

there's also MesosCoarseGrainedSchedulerBackend.scala which seems easy to fix to add support for this.

I've double checked the mentioned file but as I see it uses SparkConf to get the extra java options:

    val extraJavaOpts = conf.get(config.EXECUTOR_JAVA_OPTIONS).map {
      Utils.substituteAppNExecIds(_, appId, taskId)
    }.getOrElse("")

and SparkConf gives back the prepended value. Not yet see what can be added there.

SparkQA · 2019-07-01T13:36:38Z

Test build #107067 has finished for PR 24804 at commit f35eee3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2019-07-10T22:29:48Z

as I see it uses SparkConf to get the extra java options

You're right, I must have misread it.

vanzin · 2019-07-10T22:29:55Z

retest this please

SparkQA · 2019-07-11T01:10:47Z

Test build #107489 has finished for PR 24804 at commit f35eee3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2019-07-11T16:37:05Z

Merging to master.

rdblue · 2019-07-11T16:50:00Z

This is great! Thank you for fixing on this @gaborgsomogyi and for reviewing @vanzin and @squito!

## What changes were proposed in this pull request? This PR adds two new config properties: `spark.driver.defaultJavaOptions` and `spark.executor.defaultJavaOptions`. These are intended to be set by administrators in a file of defaults for options like JVM garbage collection algorithm. Users will still set `extraJavaOptions` properties, and both sets of JVM options will be added to start a JVM (default options are prepended to extra options). ## How was this patch tested? Existing + additional unit tests. ``` cd docs/ SKIP_API=1 jekyll build ``` Manual webpage check. Closes apache#24804 from gaborgsomogyi/SPARK-23472. Authored-by: Gabor Somogyi <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]>

This PR adds two new config properties: `spark.driver.defaultJavaOptions` and `spark.executor.defaultJavaOptions`. These are intended to be set by administrators in a file of defaults for options like JVM garbage collection algorithm. Users will still set `extraJavaOptions` properties, and both sets of JVM options will be added to start a JVM (default options are prepended to extra options). Existing + additional unit tests. ``` cd docs/ SKIP_API=1 jekyll build ``` Manual webpage check. Closes apache#24804 from gaborgsomogyi/SPARK-23472. Authored-by: Gabor Somogyi <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]> Ref: LIHADOOP-55812 RB=2325692 BUG=LIHADOOP-55812 G=spark-reviewers A=ekrogen

[SPARK-23472][CORE] Add defaultJavaOptions for driver and executor.

7402291

Client mode fix

9f41dca

Nit fix

ea50aef

dongjoon-hyun added IMPROVEMENT SPARK CORE and removed IMPROVEMENT labels Jun 12, 2019

vanzin reviewed Jun 24, 2019

View reviewed changes

Review fixes:

aeb7cce

* Removed unused config entries * Fallback test added * Fixed Mesos and Rest server property handling

Fix

a7d7327

squito reviewed Jun 27, 2019

View reviewed changes

docs/configuration.md Outdated Show resolved Hide resolved

core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala Outdated Show resolved Hide resolved

Fix

a5224fd

Fix

f08c2c6

vanzin reviewed Jun 28, 2019

View reviewed changes

docs/configuration.md Outdated Show resolved Hide resolved

Doc fix

f35eee3

vanzin closed this in f830005 Jul 11, 2019

squito mentioned this pull request Aug 7, 2019

[SPARK-27900][K8s] Add jvm oom flag #25229

Closed

[SPARK-23472][CORE] Add defaultJavaOptions for driver and executor. #24804

[SPARK-23472][CORE] Add defaultJavaOptions for driver and executor. #24804

Uh oh!

Conversation

gaborgsomogyi commented Jun 5, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gaborgsomogyi commented Jun 5, 2019

Uh oh!

SparkQA commented Jun 5, 2019

Uh oh!

vanzin commented Jun 5, 2019

Uh oh!

gaborgsomogyi commented Jun 6, 2019

Uh oh!

SparkQA commented Jun 6, 2019

Uh oh!

SparkQA commented Jun 6, 2019

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vanzin commented Jun 24, 2019

Uh oh!

SparkQA commented Jun 26, 2019

Uh oh!

SparkQA commented Jun 26, 2019

Uh oh!

squito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

gaborgsomogyi commented Jun 28, 2019

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaborgsomogyi commented Jul 1, 2019

Uh oh!

SparkQA commented Jul 1, 2019

Uh oh!

vanzin commented Jul 10, 2019

Uh oh!

vanzin commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

vanzin commented Jul 11, 2019

Uh oh!

rdblue commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rdblue commented Jul 11, 2019 •

edited

Loading