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.

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

[SPARKR][SPARK-22344] Set java.io.tmpdir for SparkR tests #19589

Closed
wants to merge 2 commits into from

Conversation

shivaram
Copy link
Contributor

This PR sets the java.io.tmpdir for CRAN checks and also disables the hsperfdata for the JVM when running CRAN checks. Together this prevents files from being left behind in /tmp

How was this patch tested?

Tested manually on a clean EC2 machine

Also disable the hsperfdata for the JVM for CRAN checks
@shivaram
Copy link
Contributor Author

cc @felixcheung -- It'll be great if you could independently test this as well !

@SparkQA
Copy link

SparkQA commented Oct 27, 2017

Test build #83132 has finished for PR 19589 at commit 2b399de.

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

@felixcheung
Copy link
Member

it looked like maybe appveyor is running too long and timed out?
can you kick it off again?

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

thanks, I will test this

@@ -18,7 +18,11 @@
context("basic tests for CRAN")

test_that("create DataFrame from list or data.frame", {
sparkR.session(master = sparkRTestMaster, enableHiveSupport = FALSE)
r_tmp_dir <- tempdir()
tmp_arg <- paste("-Djava.io.tmpdir=", r_tmp_dir, sep = "")
Copy link
Member

Choose a reason for hiding this comment

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

nit: paste0

} else {
# Disable hsperfdata on CRAN
old_java_opt <- Sys.getenv("_JAVA_OPTIONS")
Sys.setenv("_JAVA_OPTIONS"=paste("-XX:-UsePerfData", old_java_opt, sep=" "))
Copy link
Member

Choose a reason for hiding this comment

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

I think perhaps it'd better to set spark.driver.extraJavaOptions and spark.executor.extraJavaOptions (use a variable to store its value, as in sparkRTestMaster, or like in vignettes, sparkSessionConfig)

btw, I think the convention is camelCase variables :)

Copy link
Member

Choose a reason for hiding this comment

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

also sep=" " is the default

Copy link
Member

Choose a reason for hiding this comment

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

space between variable/value
"_JAVA_OPTIONS" = paste("-XX:-UsePerfData", old_java_opt, sep = " "
we should have a lintr rule that detects this at one point..

@@ -57,7 +63,7 @@ We use default settings in which it runs in local mode. It auto downloads Spark

```{r, include=FALSE}
install.spark()
sparkR.session(master = "local[1]")
sparkR.session(master = "local[1]", sparkConfig = sparkSessionConfig, enableHiveSupport = FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

hmm, good catch on enableHiveSupport I think this should be fine

sparkSessionConfig <- list(spark.driver.extraJavaOptions = tmp_arg,
spark.executor.extraJavaOptions = tmp_arg)
old_java_opt <- Sys.getenv("_JAVA_OPTIONS")
new_java_opt <- Sys.setenv("_JAVA_OPTIONS"=paste("-XX:-UsePerfData", old_java_opt, sep = " "))
Copy link
Member

Choose a reason for hiding this comment

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

don't need new_java_opt?

@SparkQA
Copy link

SparkQA commented Oct 28, 2017

Test build #83177 has finished for PR 19589 at commit 9c8afea.

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

@shivaram
Copy link
Contributor Author

Merging to master, branch-2.2 and branch-2.1

asfgit pushed a commit that referenced this pull request Oct 30, 2017
This PR sets the java.io.tmpdir for CRAN checks and also disables the hsperfdata for the JVM when running CRAN checks. Together this prevents files from being left behind in `/tmp`

## How was this patch tested?
Tested manually on a clean EC2 machine

Author: Shivaram Venkataraman <shivaram@cs.berkeley.edu>

Closes #19589 from shivaram/sparkr-tmpdir-clean.

(cherry picked from commit 1fe2761)
Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
@asfgit asfgit closed this in 1fe2761 Oct 30, 2017
asfgit pushed a commit that referenced this pull request Oct 30, 2017
This PR sets the java.io.tmpdir for CRAN checks and also disables the hsperfdata for the JVM when running CRAN checks. Together this prevents files from being left behind in `/tmp`

Tested manually on a clean EC2 machine

Author: Shivaram Venkataraman <shivaram@cs.berkeley.edu>

Closes #19589 from shivaram/sparkr-tmpdir-clean.

(cherry picked from commit 1fe2761)
Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
This PR sets the java.io.tmpdir for CRAN checks and also disables the hsperfdata for the JVM when running CRAN checks. Together this prevents files from being left behind in `/tmp`

## How was this patch tested?
Tested manually on a clean EC2 machine

Author: Shivaram Venkataraman <shivaram@cs.berkeley.edu>

Closes apache#19589 from shivaram/sparkr-tmpdir-clean.

(cherry picked from commit 1fe2761)
Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
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.

3 participants