[SPARK-4666] Improve YarnAllocator's parsing of "memory overhead" param #3525
Conversation
|
Can one of the admins verify this patch? |
|
possibly relevant to @sryza |
|
Hey @ryan-williams , mind filing a JIRA for this? |
|
In Spark 1.2, the memory overhead defaults to 7% of the executor memory. Have you noticed that you need larger than this fraction? In the change that added that fraction, there was some concern about having two different params (a constant overhead and a fraction) to control the same value. |
|
sure, will file JIRA. To answer what I think is the spirit of your question, whether a spark user wants more or less than 7% seems like something that should be configurable. I have definitely wanted to set this value to closer to 10% of the executor memory, or higher, to make sure that that wasn't my problem, when debugging some of my own Spark jobs. I can't swear that I've seen yarn kill my executors for exceeding the 7%, but I've definitely felt like I wanted to configure it. I can see that it's unusual to have two params that both affect this value, but it seems like the right tradeoff, since the fraction more closely models what we want, but by an accident of history we have this other value and we don't want to break backwards-compatibility. Everyone I've heard talk about how to think about the overhead (which is mostly you, admittedly) talks about it in terms of the fraction of the executor memory. If you think the best course is to deprecate the absolute-amount-of-memory one as well, I'm open to that, but I don't think the status quo (1. have a hard-coded value for the logical param that we care about, and 2. have a param that sets the value we want indirectly) makes much sense. |
|
there's actually two JIRAs I'll file here:
|
ryan-williams
changed the title
|
Filed SPARK-4665 and SPARK-4666, noted the former in the PR title, and put a note about both in the opening PR comment. lmk if there's some other syntax for addressing two JIRAs that is warranted. |
|
@ryan-williams your reasoning makes sense to me. @andrewor14 @tgravescs what do you think about deprecating memoryOverhead and adding memoryOverheadFraction? |
|
Perhaps I've missed it but I haven't heard a lot of cases for either way. Do you have examples or use cases? I'd be open to changing it but want more reasoning behind it. I've found putting in the value rather then a % easier in some cases that weren't small/straight forward jobs. |
|
@arahuja recently had YARN killing executors until he bumped memory-overhead to 4-5GB, on executor memory of 20-30GB, so the hard-coded 7% was not enough for him. In general, this fraction should be configurable; maybe some people want <7% too! 7% is not special, afaik. @sryza has given me the impression that the overhead tends to grow proportionally to the executor memory, meaning that allowing people to configure the fraction makes as much or more sense than having them do some division and tweak their cmd-line flag for every job in order to specify an absolute amount of memory. |
|
Basically I would like some hard data or user input before changing this since changing/adding configs can just lead to more confusion. I have seen a few cases where a % wasn't ideal. I've seen others where 7% worked just fine. Overall I'd like to understand if % covers the majority of cases or if say it is only useful doing etl but then breaks down doing ML or graphx. We had this discussion briefly before and left it at the value vs % and I haven't really seen much more data to change it but perhaps others have? |
|
so it turns out that it was actually Nishkam Ravi that made b4fb7b8, which added this 7%. Excerpts from that commit, that are live in comments and Spark documentation today:
There is a hard-coded 7% sitting in the codebase, that is "load-bearing" (dominates the other lower-bound of 384MB) above around 5GB of executor memory, with comments that say sometimes a person may want more than 7%. I understand that in general a proliferation of configuration parameters can be confusing, but IMHO it is more confusing that there are already two logical configuration parameters here, with various reasonable values for them discussed in the documentation, yet one of them not actually configurable. Also, re: user input, I am reporting, as a user, that I have had to bump |
ryan-williams
changed the title
|
FWIW I think the logic behind the existing parameters is: 7% should be enough for anyone. And if it isn't, they know it, and have a clear idea of just how much they want the overhead to be. Or: if 7% is consistently not enough, it should be higher, like 10%. I'd leave the existing param unless it's obviously problematic. But that's separate from the question here, of whether lots of these strings could be specified in mega or gigabytes. Is that still something that can move forward? the refactoring of all that seems helpful in any event. |
|
Not a huge deal, but I think the converse is kind of true. In my experience, the right percentage is totally unclear to everyone in all situations, so pretty much always needs to be set experimentally. If I settle on a percentage that works for my job (or all my jobs), it would be nice for me not to need to reset this param every time I tweak spark.executor.memory. |
|
OK maybe there's enough momentum for ... two flags? one of which overrides the other? But maybe we should also bump up the default a bit too. |
|
I'm still not on board with having 2 different configs. You then have to explain to the user what they are, which one takes presendence, etc. I can see cases the % is useful but then again you can argue the other way too. If my job needs 4G overhead, I may want to increase the executor memory without changing the overhead amount. In that case then I have to go mess with % to get it back what I want. Either way I think the user has to experiment to get the best number. I haven't seen any reason to bump up the default at this point but if others have more real world data lets consider it. How many people are complaining or having real issues with this? The main reason I'm against adding this is that I consider it an api change and we now have to support 2 configs until we can get rid of the deprecated one. Its just more dev overhead, testing, and potential user confusion. If its enough of a benefit to the user to change it then I would be ok with it but would rather deprecate the existing one in favor of the %. To me the raw number is more flexible then the % but I see the % as being easier in situations. |
|
before i think that both of two configs can be existed. but from @tgravescs i think overhead is more necessary than OverheadFraction , because at some time it has very large memory,we just use overhead to increase executor memory. now memoryOverheadFraction is very large and inappropriate. |
|
Thanks for resurrecting this, @srowen. I think there are 3 potentially separable changes here, in rough order of increasing controversial-ness:
If people prefer, I can make a separate PR with 1∪2 or individual PRs for each of 1 and 2. Re: 3, I am sympathetic to @tgravescs's arguments, but worry that we are conflating [wishing Spark's configuration surface area was smaller] with [forcing users to build custom Sparks to get at real, existing niches of said configuration surface area (in this case to change a hard-coded However, I don't mind much one way or another at this point, so I'm fine dropping 3 if that's what consensus here prefers. |
|
(until I hear otherwise I am pulling/rebasing 1∪2, roughly the SPARK-4666 bits, into their own PR) |
|
Without clear consensus at this point, and given 3 is really a question of how you like to override a default in the same way you override others, I'd do 1+2 for the moment. |
ryan-williams
force-pushed
the
mem-overhead
branch
from
5b36139 to
766e7f2
Compare
There was a problem hiding this comment.
Saw this while compiling:
[WARNING] /Users/ryan/c/spark/streaming/src/main/scala/org/apache/spark/streaming/util/WriteAheadLogManager.scala:156: postfix operator second should be enabled
by making the implicit value scala.language.postfixOps visible.
This can be achieved by adding the import clause 'import scala.language.postfixOps'
or by setting the compiler option -language:postfixOps.
See the Scala docs for value scala.language.postfixOps for a discussion
why the feature should be explicitly enabled.
There was a problem hiding this comment.
Yes that's the right thing, if you think that the use of postfix ops is justified.
ryan-williams
force-pushed
the
mem-overhead
branch
from
766e7f2 to
8959717
Compare
ryan-williams
changed the title
|
OK, this is ready to go again. Changes:
Let me know what you think. |
There was a problem hiding this comment.
Hm, why is this property special-cased here? the methods in this class are generally generic.
There was a problem hiding this comment.
I weighed two options:
- have a method in
Utilsthat takes aSparkConfas a parameter and thinly wraps a method call on saidSparkConf - make the aforementioned wrapper a method on
SparkConfthat delegates to another method onSparkConf
..and felt like the latter was better/cleaner. My feeling was that a kitchen-sink / generically-named Utils class that wraps methods for SparkConf (and possibly other classes?) to maintain an illusion of simplicity in the SparkConf API is not helping code clarity.
Of course, this is subjective and I'm open to putting it back in Utils, lmk.
There was a problem hiding this comment.
NB: that was an answer to why this property is special-cased here, as opposed to over in Utils. You may be more interested in the question of why it's special-cased at all, but that seems reasonable to me given the couple magic strings and its greater-than-1 call-sites (namely: 2).
There was a problem hiding this comment.
I would have picked Utils I suppose. Or is there somewhere less generic to put this that covers the 2 call sites? Other opinions?
There was a problem hiding this comment.
Option 3. could be: Put such methods in a SparkConfUtils object that would be less prone to kitchen-sink-iness.
I'm impartial, I'll let you make the call between these three.
There was a problem hiding this comment.
I'd like another set of eyes on the change from @pwendell or @JoshRosen . The reason I hesitate before merging is the large number of small changes and merge conflict potential. Although I do definitely think the variable names are improved by the suffix.
For this particular issue, maybe expose just getMemory from this class, and inline the two simple calls to it that currently use getMaxResultSize? writing getMemory("spark.driver.maxResultSize", "1g", outputScale = 'b') in two places isn't bad versus constructing a new home for it.
There was a problem hiding this comment.
hm, I'd vote we put it back in Utils over un-factoring those two calls that are doing the same thing as one another
Previously it assumed a unitless number represented raw bytes, but I want to use it for a config variable that previously defaulted to # of megabytes and not break backwards-compatibility.
ryan-williams
force-pushed
the
mem-overhead
branch
from
8959717 to
2ebb55a
Compare
|
OK I think it is back to you @srowen |
|
(thanks for the review!) |
|
@srowen this one ready to go in? |
|
@hammer @ryan-williams I'd prefer to hear from @JoshRosen or @pwendell before merging. The two key questions are
|
|
I think that this PR's motivation may have been addressed by #5574. @ilganeli @srowen @ryan-williams , can you confirm whether there's still an issue here and resolve this PR / JIRAs as appropriate? |
|
I think this would have to be reworked in light of SPARK-5932. I don't think that resolved this particular suggestion. Proceed that way or close this PR? |
|
@srowen @JoshRosen I think this should be refactored to use the updates from #5574 but I don't think #5574 resolves this on its own because of the need to handle the min/max allocation - my 2c. |
|
@ryan-williams would you mind opening a new PR to reflect the changes in #5574? Either that or just update this one, though the conflicts may be non-trivial since this has gotten stale. Up to you. |
|
I've lost state on this, closing, thanks all |
10 participants
fixes SPARK-4665 and SPARK-4666.