Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-9180] fix spark-shell to accept --name option #7512

Closed
wants to merge 2 commits into from
Closed

[SPARK-9180] fix spark-shell to accept --name option #7512

wants to merge 2 commits into from

Conversation

kmaehashi
Copy link
Contributor

This patch fixes [SPARK-9180].
Users can now set the app name of spark-shell using spark-shell --name "whatever".

@srowen
Copy link
Member

srowen commented Jul 19, 2015

According to SparkSubmit.scala, --name is only for YARN mode. I don't think this necessarily sets spark.app.name otherwise?

@kmaehashi
Copy link
Contributor Author

@srowen I think --name (args.name) is available for all cluster managers / deploy modes. (See: https://github.com/apache/spark/blob/v1.4.1/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L414)

@srowen
Copy link
Member

srowen commented Jul 19, 2015

Yes but https://github.com/apache/spark/blob/v1.4.1/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L434 suggests that --name (not the sys property) is recognized only for YARN? CC @sryza

@kmaehashi
Copy link
Contributor Author

@srowen

OptionAssigner(args.name, YARN, CLUSTER, clOption = "--name"),

This is a different thing. This --name is an argument to the org.apache.spark.deploy.yarn.Client class, which is a main class for YARN cluster mode. It will be parsed here: https://github.com/apache/spark/blob/v1.4.1/yarn/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala#L216

As spark-submit --help (https://github.com/apache/spark/blob/v1.4.1/core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala#L451) says, --name option (for spark-submit) is available for all cluster managers / deploy modes.

@srowen
Copy link
Member

srowen commented Jul 21, 2015

You're probably right. This makes sense if so. I'd still like to get more knowledgeable eyes on it, from @sryza or @vanzin for example.

vanzin
vanzin reviewed Jul 21, 2015
View reviewed changes
repl/scala-2.10/src/main/scala/org/apache/spark/repl/SparkILoop.scala
@@ -1008,7 +1008,7 @@ class SparkILoop(
val jars = SparkILoop.getAddedJars
val conf = new SparkConf()
.setMaster(getMaster())
.setAppName("Spark shell")
.setAppName(sys.props.getOrElse("spark.app.name", "Spark shell"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably could avoid all changes to the shell scripts and just say:

.setIfMissing("spark.app.name", "Spark shell")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanzin If --name is not specified on the spark-shell command line, the main class name (org.apache.spark.repl.Main) is set for spark.app.name, so spark.app.name will not become missing when executed via spark-shell command line:

https://github.com/apache/spark/blob/v1.4.1/core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala#L203-L204

The reason why I use getOrElse here is just for backward compatibility for special use cases (e.g., directly executing Main class of spark-shell instead of using spark-shell command line; in this case spark.app.name may not be set)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but you could still use setIfMissing instead of what you have here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK fixed as suggested.

@sarutak
Copy link
Member

sarutak commented Jul 21, 2015

ok to test.

@vanzin
Copy link
Contributor

vanzin commented Jul 21, 2015

LGTM pending tests (which shouldn't have a problem passing).

@SparkQA
Copy link

SparkQA commented Jul 21, 2015

Test build #37972 has finished for PR 7512 at commit e24991a.

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

@vanzin
Copy link
Contributor

vanzin commented Jul 21, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #45 has finished for PR 7512 at commit e24991a.

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

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #37995 has finished for PR 7512 at commit e24991a.

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

@sarutak
Copy link
Member

sarutak commented Jul 22, 2015

I think, these failures are caused by flaky tests.
retest this please.

@sarutak
Copy link
Member

sarutak commented Jul 22, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #38007 timed out for PR 7512 at commit e24991a after a configured wait of 175m.

@vanzin
Copy link
Contributor

vanzin commented Jul 22, 2015

Wow builds are really flaky.

Jenkins retest this please.

@sarutak
Copy link
Member

sarutak commented Jul 22, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #38103 has finished for PR 7512 at commit e24991a.

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

@vanzin
Copy link
Contributor

vanzin commented Jul 22, 2015

@JoshRosen any idea why this test keeps failing? Doesn't seem in any way related to the change here. Wonder if other PRs are also flaky.

@vanzin
Copy link
Contributor

vanzin commented Jul 22, 2015

Jenkins retest this please.

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #64 has finished for PR 7512 at commit e24991a.

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

@sarutak
Copy link
Member

sarutak commented Jul 22, 2015

LGTM. @vanzin I also think this is ready to merge.

@vanzin
Copy link
Contributor

vanzin commented Jul 22, 2015

Merged into master. Thanks!

@asfgit asfgit closed this in 430cd78 Jul 22, 2015
@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #38110 has finished for PR 7512 at commit e24991a.

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

@piaozhexiu piaozhexiu mentioned this pull request Jul 23, 2015
Closed
@kmaehashi kmaehashi deleted the fix-spark-shell-app-name branch July 23, 2015 07:54
asfgit pushed a commit that referenced this pull request Jul 24, 2015
[SPARK-9270] [PYSPARK] allow --name option in pyspark
9a11396 
This is continuation of #7512 which added `--name` option to spark-shell. This PR adds the same option to pyspark.

Note that `--conf spark.app.name` in command-line has no effect in spark-shell and pyspark. Instead, `--name` must be used. This is in fact inconsistency with spark-sql which doesn't accept `--name` option while it accepts `--conf spark.app.name`. I am not fixing this inconsistency in this PR. IMO, one of `--name` and `--conf spark.app.name` is needed not both. But since I cannot decide which to choose, I am not making any change here.

Author: Cheolsoo Park <cheolsoop@netflix.com>

Closes #7610 from piaozhexiu/SPARK-9270 and squashes the following commits:

763e86d [Cheolsoo Park] Update windows script
400b7f9 [Cheolsoo Park] Allow --name option to pyspark
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@kmaehashi @srowen @sarutak @vanzin @SparkQA