[SPARK-17178][SPARKR][SPARKSUBMIT] Allow to set sparkr shell command through --conf

[zjffdu]

What changes were proposed in this pull request?

Allow user to set sparkr shell command through --conf spark.r.shell.command

How was this patch tested?

Unit test is added and also verify it manually through

bin/sparkr --master yarn-client --conf spark.r.shell.command=/usr/local/bin/R

[SparkQA]

Test build #64171 has finished for PR 14744 at commit 6560ddc.

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

[zjffdu]

@vanzin Could you help review this ?

[SparkQA]

Test build #64172 has finished for PR 14744 at commit f0231f9.

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

[jerryshao]

It might be not so proper to mention "executing R shell in both client mode and cluster mode" in Spark's doc, AFAIK it is a Livy-only feature. sparkr can only run in client mode for now in Spark. So described here may confuse the user.

[zjffdu]

Make sense.

[felixcheung]

"Executable for sparkR shell"?
"Executable for R when running the sparkR shell"?

(the sparkR shell command starts with lower case s)

[sun-rui]

maybe "Executable for executing the SparkR shell. Ignored in cluster modes.". Don't have strong opinion on the wording, just be sure to be consistent with the existing description for other 2 related options. The old descriptions can be updated together.

[shivaram]

+1 would be good to explain how this is different from spark.r.driver.command

[felixcheung]

we should clarify how this works with SPARKR_DRIVER_R too

[felixcheung]

Could you open a JIRA on this and add more info on why this is needed and can't use SPARKR_DRIVER_R?

[SparkQA]

Test build #64191 has finished for PR 14744 at commit 0a24b2d.

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

[felixcheung]

@shivaram @sun-rui thought?

[sun-rui]

@felixcheung, I guess that spark conf is preferred over env variable.

[sun-rui]

@zjffdu, basically LGTM

[shivaram]

LGTM. I agree that using SparkConf is preferable over environment variables -- but it would be good to make the documentation clear on when each option is used.

[SparkQA]

Test build #64382 has finished for PR 14744 at commit fa11d2a.

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

[felixcheung]

I think you can do this instead:

args.add(firstNonEmpty(conf.get(SparkLauncher.SPARKR_R_SHELL), System.getenv("SPARKR_DRIVER_R"), "R"));

[zjffdu]

ah, right, my mistake. :(

[SparkQA]

Test build #64391 has finished for PR 14744 at commit 3303c53.

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

[SparkQA]

Test build #64413 has finished for PR 14744 at commit bb75190.

• This patch fails from timeout after a configured wait of 250m.
• This patch merges cleanly.
• This patch adds no public classes.

[zjffdu]

@shivaram @sun-rui @felixcheung Any more comments ?

[shivaram]

Jenkins, retest this please

[SparkQA]

Test build #64685 has finished for PR 14744 at commit bb75190.

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

[sun-rui]

LGTM

[felixcheung]

LGTM, thanks!