[SPARK-4697][YARN]System properties should override environment variables #3557

WangTaoTheTonic · 2014-12-02T15:10:16Z

I found some arguments in yarn module take environment variables before system properties while the latter override the former in core module.

SparkQA · 2014-12-02T15:15:11Z

Test build #24045 has started for PR 3557 at commit 5f43f45.

This patch merges cleanly.

SparkQA · 2014-12-02T16:35:38Z

Test build #24045 has finished for PR 3557 at commit 5f43f45.

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

AmplabJenkins · 2014-12-02T16:35:42Z

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

sryza · 2014-12-03T02:25:39Z

yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala

Can we maintain the structure here and just switch the blocks?

Good point, that sounds more efficient.

So this order was done on purpose. There were a few issues if they are the other way.

See commit 3f779d8 and associated jira https://issues.apache.org/jira/browse/SPARK-1631.

cc @vanzin

I know this was brought up once before but I don't know if anyone fully investigated to see if there is another solution

There have been two PRs that tried to "fix" this. The problem here is:

You cannot create a SparkContext if spark.app.name is not set in SparkConf

With this change, if spark.app.name is set, SPARK_YARN_APP_NAME will never be used.

The fix, in my view, is to stop supporting these env variables at some point.

sryza · 2014-12-03T02:25:59Z

Had one nit, otherwise this change looks right to me.

sryza · 2014-12-03T02:26:36Z

@andrewor14 @tgravescs

SparkQA · 2014-12-03T08:45:32Z

Test build #24082 has started for PR 3557 at commit 40934b4.

This patch merges cleanly.

SparkQA · 2014-12-03T10:05:04Z

Test build #24082 has finished for PR 3557 at commit 40934b4.

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

AmplabJenkins · 2014-12-03T10:05:08Z

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

andrewor14 · 2014-12-03T19:22:54Z

yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala

Hey @WangTaoTheTonic as mentioned by others this breaks the Spark app name ordering detailed here: #539. If you want to make this change then you need to special case the app name.

After taking a look at #539 and #699, I am not sure I understand what you means here. The app names observed by RM is set by appContext.setApplicationName(args.appName) while args.appName is decided by SPARK_YARN_APP_NAME than spark.app.name.
I still can't see why change this two's order could affect other aspects.

The method you're modifying (getExtraClientArguments) is the one that defines the --name argument for ClientArguments. And you're inverting the priority here, so that spark.app.name > SPARK_YARN_APP_NAME. So basically, since spark.app.name is mandatory, SPARK_YARN_APP_NAME becomes useless.

But please test it; make sure both work, both in client and cluster mode. Something might have changed since those fixes went in, although I kinda doubt it.

tgravescs · 2014-12-04T13:43:12Z

Did you test it to see if they still work?

WangTaoTheTonic · 2014-12-04T15:04:20Z

@tgravescs Ok if you mean whether the app name could still be shown correctly, I will test it.

tgravescs · 2015-01-07T15:02:47Z

@WangTaoTheTonic did you have a chance to test this?

SparkQA · 2015-01-08T12:12:36Z

Test build #25224 has started for PR 3557 at commit bee9447.

This patch merges cleanly.

SparkQA · 2015-01-08T13:22:04Z

Test build #25224 has finished for PR 3557 at commit bee9447.

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

AmplabJenkins · 2015-01-08T13:22:08Z

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

WangTaoTheTonic · 2015-01-08T13:22:35Z

I've tested it and results are:
Before this patch:

Setting	name in client mode	name in cluster mode
none	org.apache.spark.examples.SparkPi	org.apache.spark.examples.SparkPi
spark.app.name	spark.app.name	spark.app.name
SPARK_YARN_APP_NAME	SPARK_YARN_APP_NAME	org.apache.spark.examples.SparkPi
spark.app.name and SPARK_YARN_APP_NAME	SPARK_YARN_APP_NAME	spark.app.name

After this patch:

Setting	name in client mode	name in cluster mode
none	org.apache.spark.examples.SparkPi	org.apache.spark.examples.SparkPi
spark.app.name	spark.app.name	spark.app.name
SPARK_YARN_APP_NAME	org.apache.spark.examples.SparkPi	org.apache.spark.examples.SparkPi
spark.app.name and SPARK_YARN_APP_NAME	spark.app.name	spark.app.name

As @vanzin said, the SPARK_YARN_APP_NAME becomes useless since spark.app.name is mandatory.

So should we take spark.app.name as a special case or just delete SPARK_YARN_APP_NAME?

@tgravescs @andrewor14

Note: In test cases I didn't use SparkConf.setAppName in application code.

vanzin · 2015-01-08T17:51:53Z

Actually that's a little bit surprising. SparkPi.scala sets the app name to "Spark Pi", so I'd expect that to show up when the user does not explicitly override it. Also, there seems to be some disprepancies between client and cluster mode in handling the env variable.

WangTaoTheTonic · 2015-01-09T01:55:51Z

@vanzin
Note what I note :-)

Note: In test cases I didn't use SparkConf.setAppName in application code.

I didn't use the original SparkPi in sparkcode but the one I customized.

vanzin · 2015-01-09T17:58:29Z

I see. Still, the third line of the "before patch" table is unexpected.

tgravescs · 2015-01-09T18:42:45Z

where were you looking to get the application name? On the RM or in spark or both?

https://issues.apache.org/jira/browse/SPARK-3678

WangTaoTheTonic · 2015-01-10T00:38:35Z

@vanzin Which one do you mean? Client mode or Cluster mode?
@tgravescs I looked the name on RM's UI.
I checked SPARK-3678 and realized that if no spark.app.name in configuration file or --name in command args, in cluster mode it will use mainClass.
But in client mode, cause usually we use SparkConf.setAppName in application code, so on RM's UI it will show what we set.

vanzin · 2015-01-10T00:54:18Z

I mean it's unexpected because they're different when they should be the same (in that case, the value of SPARK_YARN_APP_NAME).

WangTaoTheTonic · 2015-01-10T01:22:52Z

I am afraid it is not.
In cluster mode, in SparkSubmitArguments.scala will be assigned with mainClass if spark.app.name or --name is not specified as it will not read SPARK_YARN_APP_NAME.
Then in SparkSubmit.scala it will pass args.name in format of spark.app.name and --name to org.apache.spark.deploy.yarn.Client.

yarn.Client.scala transform the args to a ClientArguments object, in which appName will only get its value from --name.

So, in the progress, the app name would never get value from the env SPARK_YARN_APP_NAME. It is only used in client mode.

vanzin · 2015-01-10T01:24:49Z

I understand all that. I'm saying that's unexpected, in that I'd expect both modes to behave the same. So if there's anything to fix here, that's it.

WangTaoTheTonic · 2015-01-10T01:35:41Z

Oh, I see. But after this patch SPARK_YARN_APP_NAME becomes useless. It will make behavior in client and cluster mode to be same.
Note that happens when we don't set app name in SparkConf. Otherwise it is a different issue described in SPARK-3678. Perhaps we should file another separate PR to solve that.

vanzin · 2015-01-10T01:36:48Z

But after this patch SPARK_YARN_APP_NAME becomes useless

That's why we should not commit this patch.

WangTaoTheTonic · 2015-01-10T01:46:06Z

Or we could just make SPARK_YARN_APP_NAME disappear? As the env variable is not recommended and it will cause different behavior. User can still use spark.app.name.

Or we should make SPARK_YARN_APP_NAME and spark.app.name a special case in YarnClientSchedulerBackend.scala ?

What do you two think? @tgravescs @andrewor14

andrewor14 · 2015-01-10T01:49:10Z

Although in general we should honor Spark properties over environment variables, the app name has been a special case and should remain so for backward compatibility (i.e. we need to keep SPARK_YARN_APP_NAME). For this PR, I think the goal is to maintain behavior in the "before" table by making more changes in YarnClientSchedulerBackend.

Additionally, it is not intuitive that if you set both SPARK_YARN_APP_NAME and spark.app.name, the behavior is inconsistent between client mode and cluster mode. I think the app name should be a special case for both deploy modes, but we can fix that in a separate PR.

SparkQA · 2015-01-12T06:57:37Z

Test build #25402 has started for PR 3557 at commit 836b9ef.

This patch merges cleanly.

WangTaoTheTonic · 2015-01-12T07:01:38Z

@andrewor14 @vanzin
After moving the logic into SparkSubmitArguments, I tested with no SparkConf.setAppName in application code, here is the results:

Setting	name in client mode	name in cluster mode
none	org.apache.spark.examples.SparkPi	org.apache.spark.examples.SparkPi
spark.app.name	spark.app.name	spark.app.name
SPARK_YARN_APP_NAME	SPARK_YARN_APP_NAME	SPARK_YARN_APP_NAME
spark.app.name and SPARK_YARN_APP_NAME	spark.app.name	spark.app.name

The names on Driver's UI and RM's UI is consistent.

SparkQA · 2015-01-12T08:05:43Z

Test build #25402 has finished for PR 3557 at commit 836b9ef.

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

AmplabJenkins · 2015-01-12T08:05:47Z

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

tgravescs · 2015-01-12T14:39:30Z

So it used to be (0.8 and 0.9) that spark on yarn was ran like this. We kept this work in the 1.0 release as well for backwards compatibility.

spark-class org.apache.spark.deploy.yarn.Client
--jar spark-examples.jar
--class org.apache.spark.examples.SparkPi --args yarn-standalone --num-workers 3 --worker-memory 2g --queue unfunded

does moving those to spark submit break the app name for this? I don't know of anyone still using that but if we break it we should consciously decide that is unsupported.

WangTaoTheTonic · 2015-01-12T15:27:59Z

I am afraid in that case user can only set app name via --name, SPARK_YARN_APP_NAME will not be used.

Eh...Sorry to ask the command you listed is using cluster mode? This PR would not impact the app name in cluster mode. In cluster mode yarn.client will get app name from --name and submit application to RM using that value.

vanzin · 2015-01-12T19:05:50Z

Personally, since there have been 3 stable releases already that have the current semantics, and we effectively treat env variables as deprecated, I'd just not change anything, document that env variables are deprecated, and remove them in a future release (1.4 or later).

But if you guys really think this is worth doing, the latest patch looks ok, I guess.

andrewor14 · 2015-01-12T19:56:28Z

@vanzin I'm not sure if it's on our roadmap to deprecate the environment variables altogether, since there are many others for other modes that are actively being used. That said I would also prefer to remove the env vars eventually, but until then we should keep the semantics in YARN consistent with other modes (i.e. system properties should override env variables). Unfortunately we can't do anything about the app name but to special case it.

@sryza @tgravescs any thoughts?

sryza · 2015-01-12T20:47:02Z

I'm strongly in favor of deprecating the env vars as well.

Until we do that, keeping the semantics consistent (and special casing if need be) seems reasonable to me.

tgravescs · 2015-01-13T17:26:54Z

I'm in favor of deprecating too but I don't think its good to do it in minor release. Probably 2.0 would be good time.

Lets just leave the app name special case.

andrewor14 · 2015-01-13T17:34:50Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala

should be startsWith, also in the comments we generally capitalize YARN

Just to be clear, you mean capitalize YARN in the comment, right?

Woops didn't see your wrote comments, sorry for the noise

andrewor14 · 2015-01-13T17:38:04Z

Hey @WangTaoTheTonic I think this LGTM. By the way, when I merge this I will file a new JIRA that makes the app name behavior consistent across cluster and client modes and add it to the commit title. I'll also fix the few minor comments I left.

This is going into master.

System property can override environment variable

5f43f45

WangTaoTheTonic changed the title ~~[SPARK-4697]System properties should override environment variables~~ [SPARK-4697][YARN]System properties should override environment variables Dec 3, 2014

sryza reviewed Dec 3, 2014
View reviewed changes

just switch blocks

40934b4

andrewor14 reviewed Dec 3, 2014
View reviewed changes

WangTaoTheTonic added 2 commits January 8, 2015 16:48

rebase

81833bb

wrong brace

bee9447

WangTaoTheTonic added 3 commits January 12, 2015 12:03

handle spark.app.name and SPARK_YARN_APP_NAME in SparkSubmitArguments

1262d57

remove the comma

e3e486a

fix type mismatch

836b9ef

andrewor14 reviewed Jan 13, 2015
View reviewed changes

asfgit closed this in 9dea64e Jan 13, 2015

andrewor14 mentioned this pull request Jun 5, 2015

[SPARK-8016] YARN cluster / client modes have different app names for python #6671

Closed

[SPARK-4697][YARN]System properties should override environment variables #3557

[SPARK-4697][YARN]System properties should override environment variables #3557

Uh oh!

Conversation

WangTaoTheTonic commented Dec 2, 2014

Uh oh!

SparkQA commented Dec 2, 2014

Uh oh!

SparkQA commented Dec 2, 2014

Uh oh!

AmplabJenkins commented Dec 2, 2014

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!

sryza commented Dec 3, 2014

Uh oh!

sryza commented Dec 3, 2014

Uh oh!

SparkQA commented Dec 3, 2014

Uh oh!

SparkQA commented Dec 3, 2014

Uh oh!

AmplabJenkins commented Dec 3, 2014

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!

tgravescs commented Dec 4, 2014

Uh oh!

WangTaoTheTonic commented Dec 4, 2014

Uh oh!

tgravescs commented Jan 7, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

AmplabJenkins commented Jan 8, 2015

Uh oh!

WangTaoTheTonic commented Jan 8, 2015

Uh oh!

vanzin commented Jan 8, 2015

Uh oh!

WangTaoTheTonic commented Jan 9, 2015

Uh oh!

vanzin commented Jan 9, 2015

Uh oh!

tgravescs commented Jan 9, 2015

Uh oh!

WangTaoTheTonic commented Jan 10, 2015

Uh oh!

vanzin commented Jan 10, 2015

Uh oh!

WangTaoTheTonic commented Jan 10, 2015

Uh oh!

vanzin commented Jan 10, 2015

Uh oh!

WangTaoTheTonic commented Jan 10, 2015

Uh oh!

vanzin commented Jan 10, 2015

Uh oh!

WangTaoTheTonic commented Jan 10, 2015

Uh oh!

andrewor14 commented Jan 10, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!

WangTaoTheTonic commented Jan 12, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!