-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-4666] Improve YarnAllocator's parsing of "memory overhead" param #3525
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
3e1cc29
rename: executorMemory -> executorMemoryMB
ryan-williams cba802c
rename: amMemory -> amMemoryMB
ryan-williams 0a717d7
rename: driverMemory -> driverMemoryMB
ryan-williams 6635945
rename: DEFAULT_MEMORY -> DEFAULT_MEMORY_MB
ryan-williams 96dfec0
rename: amMemoryOverhead -> amMemoryOverheadMB
ryan-williams 5a3e4b8
rename: executorMemoryOverhead -> executorMemoryOverheadMB
ryan-williams 63086eb
rename: amMemoryOverhead -> amMemoryOverheadMB
ryan-williams f54b0ce
rename: executorMemoryOverhead -> executorMemoryOverheadMB
ryan-williams fa3d69f
rename: MEMORY_OVERHEAD_MIN -> MEMORY_OVERHEAD_MIN_MB
ryan-williams f265d15
fix deprecation warning
ryan-williams c29da1d
rename: executorMemory -> executorMemoryMB
ryan-williams 23a77be
rename: executorMemory -> executorMemoryMB
ryan-williams 5057bd3
rename: memoryOverhead -> memoryOverheadMB
ryan-williams 14bd3d5
rename: memory -> memoryMB
ryan-williams 6e69b08
rename: mem -> memMB
ryan-williams 48c5115
memoryStringToMb can have default scale specified
ryan-williams dc03bf2
move getMaxResultSize from Utils to SparkConf
ryan-williams 40ac6ce
privatize amMemoryOverheadConf
ryan-williams dd9be85
refactor memory-size order-of-magnitude constants
ryan-williams bb66b22
add memory-string-parsing helpers to Utils
ryan-williams 960b525
add `getMemory`, `getMB` helpers to SparkConf
ryan-williams e038867
use SparkConf.getMB helper in Yarn memory parsing
ryan-williams 2ebb55a
update docs about YARN memory overhead parameters
ryan-williams 50f0f52
code review feedback
ryan-williams File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
add memory-string-parsing helpers to Utils
- Loading branch information
commit bb66b222745a85477f32ce03bb72f2e452e5a670
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why is a for construction used here? just to handle the invalid in / out scale param? My personal taste would be to just check that the Option[Long] exists directly and do away with it. I can't tell how much that's just me versus how the kids talk in Scala these days. A looping construct surprised me as there is no loop.
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.
forcomprehensions are commonly used whenmaping over 2 or more monads to avoid arguably-ugly.flatMap-chaining; the syntax just removes some boilerplate, e.g.:vs.
(I collapsed the
scalewrapper line in the latter for apples-to-apples brevity, and can do that in the PR as well).So, it's not a "looping construct" so much as a "mapping" one, commonly used on
Options,Lists, and even things like twitterFutures (search for "for {").However, it does get better as the number of chained
maps increases, e.g. especially when there are 3 or more, so I'm not that tied to it here.Of course, handling such situations using a
matchis also possible:I prefer all of these to, say, the following straw-man that doesn't take advantage of any of the nice things that come from using
Options:but I'll defer to you on the approach you like best.
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.
Yeah I get what it does. I was comparing in my mind to...
Here's another instance where I wouldn't mind hearing another opinion as it's a good more general style question.