You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rdblue <gi...@git.apache.org> on 2018/02/20 22:17:38 UTC
[GitHub] spark pull request #20645: SPARK-23472: Add defaultJavaOptions for drivers a...
GitHub user rdblue opened a pull request:
https://github.com/apache/spark/pull/20645
SPARK-23472: Add defaultJavaOptions for drivers and executors.
## What changes were proposed in this pull request?
This 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. Otherwise, it is difficult for administrators to add properties that are not clobbered when users change their JVM options.
## How was this patch tested?
We have had this deployed to production for several weeks. If there is a place to add command-line options tests, please let me know and I'll add them.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rdblue/spark SPARK-23476-add-default-jvm-options
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20645.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20645
----
commit 89ee1f330047c6c8a986e62e8201d9b7890cf4c8
Author: Ryan Blue <bl...@...>
Date: 2018-01-29T18:59:10Z
SPARK-23472: Add defaultJavaOptions for drivers and executors.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20645
I agree it would be nicer to have this be a more general feature. I would prefer an approach which didn't require a different configuration name, just as its more to document & for users to keep track of. An alternative would be to have another syntax for appending to an options, perhaps ++= like scala , eg. "--conf spark.executor.extraJavaOptions++=..."
You'd still need to tag the ConfigBuilder with what separator to use to merge the values together.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/20645
> A quick question: after this change, extraJavaOptions is still able to cleanly override whatever's set in defaultJavaOptions, is that right?
No, the intent is for both sets of options to be passed. How the JVM interprets the options is not up to Spark. This is intended to provide a way for administrators to default properties so they are not accidentally overridden when a user adds `--driver-java-options`. Users can still override `defaultJavaOptions` if they need to deviate from adminstrator defaults.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by rednaxelafx <gi...@git.apache.org>.
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20645
A quick question: after this change, `extraJavaOptions` is still able to cleanly override whatever's set in `defaultJavaOptions`, is that right?
^^ Just making sure I understood the intent correctly and not the other way around. There may well be the other side of administrative needs which is to "force options", e.g. force `-XX:-DisableExplicitGC` so that NIO direct memory doesn't get into trouble, but that'd be out of scope of this PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20645
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87569/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20645
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/975/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/20645
I like the `ConfigBuilder` approach. That would make this much more useful. I'll add an implementation like that.
I think append option syntax would be confusing for users and administrators. We need to make sure it is not easy to overwrite the default options and we can't expect users to know they need to append, and how to do it. Administrators could add `++=` options, but then we would be applying these in the wrong order to get the behavior we're after, which is bad because order matters in some cases. I like the two-property approach better because it is simpler and more predictable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20645
I like the functionality, not necessarily the implementation. Other configs would benefit from this, and having this functionality more easily available without having to change the call sites would make it more useful.
I'm also not a big fan of "default" being used in the name. But can't really think of a better alternative at the moment.
As far as implementation, I think building this into the `ConfigBuilder` code would be better. e.g. add a method where you can configure a property name for the "default" value, and when reading a property, both configs would be concatenated with some configurable separator. e.g., using your names:
```
private[spark] val DRIVER_JAVA_OPTIONS =
ConfigBuilder(SparkLauncher.DRIVER_EXTRA_JAVA_OPTIONS)
.withDefaultConfig(SparkLauncher.DRIVER_DEFAULT_JAVA_OPTIONS, " ")
.stringConf
.createOptional
```
And reading `DRIVER_JAVA_OPTIONS` would return the two configs, concatenated, separated by a space. Then you could use this for things like classpath, native libraries, and many others without having to change any of the code that reads those configs.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20645
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20645
**[Test build #87569 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87569/testReport)** for PR 20645 at commit [`89ee1f3`](https://github.com/apache/spark/commit/89ee1f330047c6c8a986e62e8201d9b7890cf4c8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20645
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20645
**[Test build #87569 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87569/testReport)** for PR 20645 at commit [`89ee1f3`](https://github.com/apache/spark/commit/89ee1f330047c6c8a986e62e8201d9b7890cf4c8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/20645
@vanzin, you might be interested in this one because it makes applying administrator settings for Spark much easier.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org