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

[witgo]

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

[SparkQA]

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

• This patch merges cleanly.

[SparkQA]

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]

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]

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]

@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]

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

[vanzin]

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]

Yes thanks you would want to use that one.

[witgo]

@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]

@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]

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

[andrewor14]

@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]

QA tests have started for PR 2379 at commit dae0120.

• This patch merges cleanly.

[SparkQA]

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]

OK, the code has been updated.

[SparkQA]

QA tests have started for PR 2379 at commit c1824d2.

• This patch merges cleanly.

[SparkQA]

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]

QA tests have started for PR 2379 at commit c4a77e9.

• This patch merges cleanly.

[SparkQA]

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

• This patch merges cleanly.

[vanzin]

nit: space before }

[vanzin]

LGTM, just a couple of minor things.

[SparkQA]

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]

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]

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

[andrewor14]

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

[SparkQA]

QA tests have started for PR 2379 at commit e1bb650.

• This patch merges cleanly.

[SparkQA]

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]

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]

@andrewor14 The code has been updated.

[andrewor14]

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]

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

[SparkQA]

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

• This patch merges cleanly.

[SparkQA]

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]

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]

@andrewor14 The code has been updated.

[andrewor14]

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

[witgo]

Cool! Thanks @andrewor14@vanzin