Skip to content

Conversation

@felixcheung
Copy link
Member

Mapping spark.driver.memory from sparkEnvir to spark-submit commandline arguments.

@shivaram suggested that we possibly add other spark.driver.* properties - do we want to add all of those? I thought those could be set in SparkConf?
@sun-rui

@felixcheung
Copy link
Member Author

Manual testing with:

library(SparkR)
sc <- sparkR.init(master = "local[*]", sparkEnvir = list(spark.driver.memory = "2g"))

before
image

after
image

@felixcheung
Copy link
Member Author

I checked, the user could also set SPARK_DRIVER_MEMORY before running sparkR.init()
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala#L157

@SparkQA
Copy link

SparkQA commented Oct 27, 2015

Test build #44405 has finished for PR 9290 at commit d3f8d28.

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

@SparkQA
Copy link

SparkQA commented Oct 27, 2015

Test build #44409 has finished for PR 9290 at commit 5ecc9e0.

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

R/pkg/R/sparkR.R Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add the support for other options of the same limitation, according to http://spark.apache.org/docs/latest/configuration.html, we also have:
spark.driver.extraClassPath
spark.driver.extraJavaOptions
spark.driver.extraLibraryPath

…aLibraryPath from feedback

add quote " around parameter values
@SparkQA
Copy link

SparkQA commented Oct 28, 2015

Test build #44490 has finished for PR 9290 at commit db8f7fd.

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

@SparkQA
Copy link

SparkQA commented Oct 28, 2015

Test build #44494 has finished for PR 9290 at commit 2d4ffb0.

  • This patch fails R style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 28, 2015

Test build #44509 has finished for PR 9290 at commit ceeca81.

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

R/pkg/R/sparkR.R Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

wording:
A few Spark properties can not take effect after the driver JVM has started, as documented in
http://spark.apache.org/docs/latest/configuration.html#application-properties

When starting SparkR without using spark-submit, for example, in Rstudio, add them to spark-submit commandline if not already set in SPARKR_SUBMIT_ARGS so that they can take effect when launching the JVM.

@sun-rui
Copy link
Contributor

sun-rui commented Oct 28, 2015

code looks good.

Could you update the SparkR documentation to document this support?

@SparkQA
Copy link

SparkQA commented Oct 28, 2015

Test build #44547 has finished for PR 9290 at commit c21713e.

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

@SparkQA
Copy link

SparkQA commented Oct 29, 2015

Test build #44552 has finished for PR 9290 at commit 557bbc1.

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

R/pkg/R/sparkR.R Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding example here may imply that "spark.driver.memory" always works. Maybe move this line to be near with the code change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could take this out - this is more for API doc/roxygen. If we are moving it out it would be visible only when someone is reading the code. Maybe pointing to the SparkR programming guide is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we should take this out of here. I think we should add a new section in the Programming guide called 'Running SparkR from RStudio' and include this option there. Here we could just have a link to the latest programming guide with something like 'For more options on how to initialize and use SparkR see our programming guide'

Copy link
Member Author

Choose a reason for hiding this comment

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

updated. created JIRA for additional programming guide change: https://issues.apache.org/jira/browse/SPARK-11407

I could take a shot at the doc change if you'd like

@SparkQA
Copy link

SparkQA commented Oct 29, 2015

Test build #44624 has finished for PR 9290 at commit 871e971.

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

@shivaram
Copy link
Contributor

LGTM. Thanks @felixcheung -- Merging this.

@asfgit asfgit closed this in bb5a2af Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants