-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARKR][SPARK-10711] Do not assume spark.submit.deployMode is always set #8832
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
Conversation
|
cc @shivaram |
|
Any reason this would not be set ? My assumption was that all spark-submit applications had this -- so I guess this is for applications not using spark-submit ? cc @sun-rui who added the function |
|
Yes, if an application starts the JVM manually (not using spark-submit) this call will fail. The change just adds safety without compromising functionality or correctness. |
|
Test build #42699 has finished for PR 8832 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if anything the default deploy mode should be client to be consistent with assumptions in the code elsewhere
|
@falaki , are you using RStudio, where spark-submit is not involved? I don't think we can simply set a default value. Because it is possible that the default mode does not match spark master. For example, if the spark master is 'yarn-cluster', the default mode is "client", that does not match. The correct policy is: I think this issue is not related to SparkR only. @andrewor14, any policy when handling default configurations when a spark-application is not launched via spark-submit? Should we extract some logic from spark-submit so that the logic can be called in non-spark-submit cases to keep consistency? |
|
retest this please |
We're actually deprecating the master URLs
We do have the launcher library, but beyond that I don't think this is something we support because it gets pretty difficult to maintain. AFAIK deploy mode is the only such config so this should be fine. |
|
LGTM merging once tests pass |
|
Test build #42800 has finished for PR 8832 at commit
|
|
I've merged this. |
…s set
In ```RUtils.sparkRPackagePath()``` we
1. Call ``` sys.props("spark.submit.deployMode")``` which returns null if ```spark.submit.deployMode``` is not suet
2. Call ``` sparkConf.get("spark.submit.deployMode")``` which throws ```NoSuchElementException``` if ```spark.submit.deployMode``` is not set. This patch simply passes a default value ("cluster") for ```spark.submit.deployMode```.
cc rxin
Author: Hossein <[email protected]>
Closes #8832 from falaki/SPARK-10711.
(cherry picked from commit c986e93)
Signed-off-by: Reynold Xin <[email protected]>
In
RUtils.sparkRPackagePath()wesys.props("spark.submit.deployMode")which returns null ifspark.submit.deployModeis not suetsparkConf.get("spark.submit.deployMode")which throwsNoSuchElementExceptionifspark.submit.deployModeis not set. This patch simply passes a default value ("cluster") forspark.submit.deployMode.cc @rxin