-
Notifications
You must be signed in to change notification settings - Fork 28.9k
SPARK-5570: No docs stating that `new SparkConf().set("spark.driver.memory", ...) will not work #4665
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
|
Test build #27663 has started for PR 4665 at commit
|
|
Test build #27663 has finished for PR 4665 at commit
|
|
Test PASSed. |
|
Hey @ilganeli thanks for doing this. Can you also do this for the other |
|
Sure @andrewor14 , I presume their behavior is identical ? |
|
Yes, those won't work either if the user sets them through the conf. |
docs/configuration.md
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.
By the way this explanation isn't exactly true. It only doesn't work if you set them through the conf.set(...) way. It still works if you put them in your conf/spark-defaults.conf file.
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.
Got it - thanks for clarifying that. I'll make the relevant changes and re-submit a commit.
|
Hi @andrewor14 - Can you please review the language for the other driver options and let me know if you agree with it? Thanks! |
|
Test build #27736 has started for PR 4665 at commit
|
|
Test build #27736 has finished for PR 4665 at commit
|
|
Test FAILed. |
|
What is a "MiMa test" failure?? |
|
MiMa is the check for changing public API signatures. This is a false positive since you didn't change any code, so don't worry about it, although a rebase might clear it up anyway. Also, this may be purely a matter of taste, but I might have done |
|
Got it - I wanted to break up the text. That's a good idea. |
|
Test build #27752 has started for PR 4665 at commit
|
docs/configuration.md
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.
what does (e.g. YARN deployment) mean? I would just rephrase this whole thing with:
<br/>Note: In client mode, this config must not be set through the <code>SparkConf</code>
directly in your application, because the driver JVM has already started at that point.
Instead, please set this through the <code>--driver-memory</code> command line option
or in your default properties file.
Can you update other places with the wording suggested?
|
Test build #27754 has started for PR 4665 at commit
|
docs/configuration.md
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.
you forgot to replace this one too
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.
actually, this one doesn't apply here. Instead, you should just replace this one with
This is used in cluster mode only.
docs/configuration.md
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.
same here, --driver-library-path
|
Test build #27755 has started for PR 4665 at commit
|
|
Hi @ilganeli thanks for reiterating on my comments quickly. I think this is pretty close to being merged. |
|
I'm happy to help :) |
|
Test build #27756 has started for PR 4665 at commit
|
|
Ok, LGTM I'm merging this into master and 1.3 thanks. |
…emory", ...) will not work I've updated documentation to reflect true behavior of this setting in client vs. cluster mode. Author: Ilya Ganelin <[email protected]> Closes #4665 from ilganeli/SPARK-5570 and squashes the following commits: 5d1c8dd [Ilya Ganelin] Added example configuration code a51700a [Ilya Ganelin] Getting rid of extra spaces 85f7a08 [Ilya Ganelin] Reworded note 5889d43 [Ilya Ganelin] Formatting adjustment f149ba1 [Ilya Ganelin] Minor updates 1fec7a5 [Ilya Ganelin] Updated to add clarification for other driver properties db47595 [Ilya Ganelin] Slight formatting update c899564 [Ilya Ganelin] Merge remote-tracking branch 'upstream/master' into SPARK-5570 17b751d [Ilya Ganelin] Updated documentation for driver-memory to reflect its true behavior in client vs cluster mode (cherry picked from commit 6bddc40) Signed-off-by: Andrew Or <[email protected]>
|
Test build #27752 has finished for PR 4665 at commit
|
|
Test PASSed. |
|
Test build #27755 has finished for PR 4665 at commit
|
|
Test FAILed. |
|
Test build #27754 has finished for PR 4665 at commit
|
|
Test PASSed. |
|
Test build #27756 has finished for PR 4665 at commit
|
|
Test PASSed. |
I've updated documentation to reflect true behavior of this setting in client vs. cluster mode.