[SPARK-2098] All Spark processes should support spark-defaults.conf, config file #2379

witgo · 2014-09-13T07:37:47Z

This is another implementation about #1256
cc @andrewor14 @vanzin

SparkQA · 2014-09-13T07:44:13Z

QA tests have started for PR 2379 at commit 217280a.

This patch merges cleanly.

SparkQA · 2014-09-13T08:54:49Z

QA tests have finished for PR 2379 at commit 217280a.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class JavaSparkContext(val sc: SparkContext)
- class JavaStreamingContext(val ssc: StreamingContext) extends Closeable

vanzin · 2014-09-16T16:01:38Z

I think I have an opposite view from Andrew in that I dislike using sys.props as an IPC mechanism, but other than that, looks good.

vanzin · 2014-09-16T16:04:25Z

core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala

-    initSecurity()
-    new HistoryServerArguments(conf, argStrings)
+    val arg = new HistoryServerArguments(argStrings)
+    Option(arg.propertiesFile).foreach { file =>


This block is repeated 3 times, maybe add it to Utils? Also, wonder if this would look better:

Option(arg.propertiesFile) .map { f => Utils.getPropertiesFromFile(f) } .foreach { props => props.filter { case (k, v) => k.startsWith("spark.") } .foreach { case (k, v) => sys.props.getOrElseUpdate(k, v) } }

andrewor14 · 2014-09-16T17:08:50Z

@vanzin @witgo Actually my intention was not to put everything you load from the properties file into sys.props. This is what SparkSubmit does, but only because it does not have access to the conf that the application will use. For Worker, Master, HistoryServer, however, this is not true; we create a SparkConf from the beginning and we will use that for the entire process. The way we set the properties through sys.props right now makes the initialization ordering of SparkConf and the respective *Argument class sensitive. Instead, it might be simpler if we do conf.set for those places instead (we already do this elsewhere in HistoryServerArgument).

vanzin · 2014-09-16T17:11:23Z

@andrewor14 makes sense. Yeah, that would simplify the change a lot.

vanzin · 2014-09-16T17:13:00Z

Just to make sure, a blind conf.set would override anything the use has set using system properties, which is probably not what we want. Probably beter to use conf.setIfMissing.

andrewor14 · 2014-09-16T17:20:14Z

Yes thanks you would want to use that one.

witgo · 2014-09-17T01:41:19Z

@andrewor14 @vanzin This is a very good idea, we create a SparkConf from the beginning and we will use that for the entire process. But it need to modify a lot of code ,Maybe we need to create a separate jira

andrewor14 · 2014-09-17T04:41:34Z

@witgo Not sure if I understand what you mean. The changes I am proposing actually changes less code (compared to Spark master), not more. Right now a problem in this PR is that the ordering between new SparkConf and new WorkerArgument is changed, but I'm saying it should not be. This is solving the same issue, so I don't see why we should create a new JIRA.

andrewor14 · 2014-09-17T04:45:19Z

core/src/main/scala/org/apache/spark/deploy/worker/WorkerArguments.scala

@@ -25,7 +25,7 @@ import org.apache.spark.SparkConf
 /**
 * Command-line parser for the worker.
 */
-private[spark] class WorkerArguments(args: Array[String], conf: SparkConf) {


As mentioned elsewhere, we shouldn't remove SparkConf as an argument.

andrewor14 · 2014-09-17T04:46:31Z

@witgo I have added a few more in-line comments to explain what I mean. Let me know if you have any more questions.

SparkQA · 2014-09-17T16:04:18Z

QA tests have started for PR 2379 at commit dae0120.

This patch merges cleanly.

SparkQA · 2014-09-17T17:10:19Z

QA tests have finished for PR 2379 at commit dae0120.

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

witgo · 2014-09-18T00:57:11Z

OK, the code has been updated.

SparkQA · 2014-09-18T00:59:27Z

QA tests have started for PR 2379 at commit c1824d2.

This patch merges cleanly.

SparkQA · 2014-09-18T02:05:18Z

QA tests have finished for PR 2379 at commit c1824d2.

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

SparkQA · 2014-09-18T02:24:27Z

QA tests have started for PR 2379 at commit c4a77e9.

This patch merges cleanly.

SparkQA · 2014-10-07T20:59:40Z

QA tests have started for PR 2379 at commit 80b0b12.

This patch merges cleanly.

vanzin · 2014-10-07T21:00:18Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+  def getDefaultPropertiesFile(): String = {
+    sys.env.get("SPARK_CONF_DIR")
+      .orElse(sys.env.get("SPARK_HOME").map { t => s"$t${File.separator}conf"})
+      .map { t => new File(s"$t${File.separator}spark-defaults.conf")}


nit: space before }

vanzin · 2014-10-07T21:00:55Z

LGTM, just a couple of minor things.

SparkQA · 2014-10-07T21:50:42Z

QA tests have finished for PR 2379 at commit 80b0b12.

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

AmplabJenkins · 2014-10-07T21:50:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21404/Test FAILed.

andrewor14 · 2014-10-07T22:05:39Z

Looks like there is a legitimate test failure though, can you resolve this @witgo?

andrewor14 · 2014-10-07T22:11:23Z

I just tested this on the Worker, Master and the HistoryServer and it works as expected. Awesome.

SparkQA · 2014-10-08T02:49:45Z

QA tests have started for PR 2379 at commit e1bb650.

This patch merges cleanly.

SparkQA · 2014-10-08T03:55:06Z

QA tests have finished for PR 2379 at commit e1bb650.

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

AmplabJenkins · 2014-10-08T03:55:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21441/Test PASSed.

witgo · 2014-10-08T04:06:05Z

@andrewor14 The code has been updated.

andrewor14 · 2014-10-08T20:14:58Z

Thanks, I will look at this shortly. A minor request though: in the future could you keep around all the commits in your branch? It would help reviewing if I could see what lines have been changed in a particular commit that was pushed after my comments, for example.

andrewor14 · 2014-10-14T21:23:03Z

Hey sorry @witgo there are merge conflicts now. I'll test this again and merge this once you update it.

SparkQA · 2014-10-15T01:49:38Z

QA tests have started for PR 2379 at commit 4ef1cbd.

This patch merges cleanly.

SparkQA · 2014-10-15T02:57:04Z

QA tests have finished for PR 2379 at commit 4ef1cbd.

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

AmplabJenkins · 2014-10-15T02:57:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21759/
Test PASSed.

witgo · 2014-10-15T03:40:42Z

@andrewor14 The code has been updated.

andrewor14 · 2014-10-15T05:16:12Z

Alright, I'm merging this into master. Thanks @witgo!

witgo · 2014-10-15T05:23:38Z

Cool! Thanks @andrewor14 @vanzin

vanzin reviewed Sep 16, 2014
View reviewed changes

andrewor14 reviewed Sep 17, 2014
View reviewed changes

witgo force-pushed the SPARK-2098-new branch from 217280a to dae0120 Compare September 17, 2014 16:01

witgo force-pushed the SPARK-2098-new branch from c1824d2 to c4a77e9 Compare September 18, 2014 02:19

vanzin reviewed Oct 7, 2014
View reviewed changes

witgo force-pushed the SPARK-2098-new branch from 80b0b12 to e1bb650 Compare October 8, 2014 02:44

andrewor14 mentioned this pull request Oct 8, 2014

Minor change in the comment of spark-defaults.conf.template #2709

Closed

witgo added 3 commits October 15, 2014 09:45

All Spark processes should support spark-defaults.conf, config file

c45d20c

Refactor getDefaultPropertiesFile

49ef70e

review commit

4ef1cbd

witgo force-pushed the SPARK-2098-new branch from e1bb650 to 4ef1cbd Compare October 15, 2014 01:47

asfgit closed this in 293a0b5 Oct 15, 2014

witgo deleted the SPARK-2098-new branch October 15, 2014 05:22

gatorsmile mentioned this pull request Aug 1, 2017

[SPARK-21637][SPARK-21451][SQL]get spark.hadoop.* properties from sysProps to hiveconf #18668

Closed

gerashegalov mentioned this pull request Aug 24, 2018

[SPARK-25221][DEPLOY] Consistent trailing whitespace treatment of conf values #22213

Closed

[SPARK-2098] All Spark processes should support spark-defaults.conf, config file #2379

[SPARK-2098] All Spark processes should support spark-defaults.conf, config file #2379

Conversation

witgo commented Sep 13, 2014

SparkQA commented Sep 13, 2014

SparkQA commented Sep 13, 2014

vanzin commented Sep 16, 2014

vanzin Sep 16, 2014

Choose a reason for hiding this comment

andrewor14 commented Sep 16, 2014

vanzin commented Sep 16, 2014

vanzin commented Sep 16, 2014

andrewor14 commented Sep 16, 2014

witgo commented Sep 17, 2014

andrewor14 commented Sep 17, 2014

andrewor14 Sep 17, 2014

Choose a reason for hiding this comment

andrewor14 commented Sep 17, 2014

SparkQA commented Sep 17, 2014

SparkQA commented Sep 17, 2014

witgo commented Sep 18, 2014

SparkQA commented Sep 18, 2014

SparkQA commented Sep 18, 2014

SparkQA commented Sep 18, 2014

SparkQA commented Oct 7, 2014

vanzin Oct 7, 2014

Choose a reason for hiding this comment

vanzin commented Oct 7, 2014

SparkQA commented Oct 7, 2014

AmplabJenkins commented Oct 7, 2014

andrewor14 commented Oct 7, 2014

andrewor14 commented Oct 7, 2014

SparkQA commented Oct 8, 2014

SparkQA commented Oct 8, 2014

AmplabJenkins commented Oct 8, 2014

witgo commented Oct 8, 2014

andrewor14 commented Oct 8, 2014

andrewor14 commented Oct 14, 2014

SparkQA commented Oct 15, 2014

SparkQA commented Oct 15, 2014

AmplabJenkins commented Oct 15, 2014

witgo commented Oct 15, 2014

andrewor14 commented Oct 15, 2014

witgo commented Oct 15, 2014