-
Notifications
You must be signed in to change notification settings - Fork 28.9k
SPARK-1652: Set driver memory correctly in spark-submit. #730
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
The previous check didn't account for the fact that the default deploy mode is "client" unless otherwise specified. Also, this sets the more narrowly defined SPARK_DRIVER_MEM instead of setting SPARK_MEM.
|
Merged build triggered. |
|
Merged build started. |
|
This was reported by @witgo. |
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
|
I'm actually a little nervous about the default value of DEPLOY_MODE. Do you think we could put an explicit line in the shell script like We can keep the rest of your solution as-is, I just like having a 2-valued DEPLOY_MODE better than a 3-valued one. |
|
@aarondav - would never want to make you nervous. I made the suggested change. Mind taking a look? |
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
bin/spark-submit
Outdated
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.
Sorry to not notice this earlier,but given the default value, I think the equality check with "client" now may make more sense.
Also, along similar lines, perhaps we could use -n instead of ! -z for DRIVER_MEMORY, and also wrap the variables in quotes. For instance, right now if $DRIVER_MEMORY is not set, this is actually evaluating if [ ! -z ], which happens to evaluate to false, but is confusing because if [ ! -n ] also evaluates to false. By putting quotes around it, we'll make sure we actually evaluate if [ -n "" ] if it's empty, which has the semantics we're actually looking for.
|
@aarondav check again? Responded to your feedback. |
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
|
LGTM, merged into master and branch-1.0. Thanks!! |
The previous check didn't account for the fact that the default deploy mode is "client" unless otherwise specified. Also, this sets the more narrowly defined SPARK_DRIVER_MEMORY instead of setting SPARK_MEM. Author: Patrick Wendell <[email protected]> Closes #730 from pwendell/spark-submit and squashes the following commits: 430b98f [Patrick Wendell] Feedback from Aaron e788edf [Patrick Wendell] Changes based on Aaron's feedback f508146 [Patrick Wendell] SPARK-1652: Set driver memory correctly in spark-submit. (cherry picked from commit 05c9aa9) Signed-off-by: Aaron Davidson <[email protected]>
The previous check didn't account for the fact that the default deploy mode is "client" unless otherwise specified. Also, this sets the more narrowly defined SPARK_DRIVER_MEMORY instead of setting SPARK_MEM. Author: Patrick Wendell <[email protected]> Closes apache#730 from pwendell/spark-submit and squashes the following commits: 430b98f [Patrick Wendell] Feedback from Aaron e788edf [Patrick Wendell] Changes based on Aaron's feedback f508146 [Patrick Wendell] SPARK-1652: Set driver memory correctly in spark-submit.
The previous check didn't account for the fact that the default
deploy mode is "client" unless otherwise specified. Also, this
sets the more narrowly defined SPARK_DRIVER_MEMORY instead of setting
SPARK_MEM.