Skip to content

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Aug 20, 2016

This PR is to unzipped sparkr.zip on executor side.

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 20, 2016

@vanzin Although this PR can make it work, but it needs some configuration change on spark-defaults.conf. Because livy use R to launch R script while spark use Rscript, is there any reason why livy doesn't use Rscript ?

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 20, 2016

Never mind, I think I may made a mistake in #177. R is used in spark as well for interactive mode while RScript is used for batch mode.

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 22, 2016

@vanzin I have a Spark PR (apache/spark#14744) to allow setting sparkr shell command through configuration. And I updated this livy PR based on the spark PR.

@codecov-io
Copy link

codecov-io commented Aug 22, 2016

Current coverage is 69.18% (diff: 50.00%)

Merging #181 into master will decrease coverage by 0.68%

@@             master       #181   diff @@
==========================================
  Files            79         81     +2   
  Lines          3867       3956    +89   
  Methods           0          0          
  Messages          0          0          
  Branches        645        664    +19   
==========================================
+ Hits           2702       2737    +35   
- Misses          792        834    +42   
- Partials        373        385    +12   

Powered by Codecov. Last update 6a32fd5...c5415c2

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 26, 2016

ping @vanzin

val rArchivesFile = new File(path)
require(rArchivesFile.exists(), "sparkr.zip not found; cannot run sparkr application.")
rArchivesFile.getAbsolutePath()
rArchivesFile.getAbsolutePath() + "#sparkr"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to add this suffix when the user sets SPARKR_PACKAGE too? Avoids having to force the user to do it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, right.

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 6, 2016

ping @vanzin

@alex-the-man alex-the-man force-pushed the master branch 2 times, most recently from 39a5162 to 2d6e026 Compare September 7, 2016 14:23
@vanzin
Copy link
Contributor

vanzin commented Sep 7, 2016

Merging.

@vanzin vanzin closed this in 0284148 Sep 7, 2016
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