[SPARK-19522] Fix executor memory in local-cluster mode #16975
Conversation
|
Test build #73058 has finished for PR 16975 at commit
|
| @@ -466,7 +466,7 @@ object SparkSubmit extends CommandLineUtils { | |||
| // Other options | |||
| OptionAssigner(args.executorCores, STANDALONE | YARN, ALL_DEPLOY_MODES, | |||
| sysProp = "spark.executor.cores"), | |||
| OptionAssigner(args.executorMemory, STANDALONE | MESOS | YARN, ALL_DEPLOY_MODES, | |||
| OptionAssigner(args.executorMemory, ALL_CLUSTER_MGRS, ALL_DEPLOY_MODES, | |||
There was a problem hiding this comment.
Is the change in SparkContext needed? Seems like this should be all that's needed.
As far as I understand, the last value in the local-cluster master is the amount of memory the worker has available; you may, for whatever reason, want to run executors with less than that, which your change doesn't seem to allow.
There was a problem hiding this comment.
If this were the only change then specifying local-cluster[2,1,2048] doesn't actually do anything because we're not setting spark.executor.memory=2048mb anywhere. You could do --master local-cluster[2,1,2048] --conf spark.executor.memory=2048mb but that's cumbersome and now there are two ways to set the executor memory.
There was a problem hiding this comment.
You may, for whatever reason, want to run executors with less than that, which your change doesn't seem to allow.
Yeah, I thought about this long and hard but I just couldn't come up with a case where you would possibly want the worker size to be different from executor size in local-cluster mode. If you want to launch 5 workers (2GB), each with 2 executors (1GB), then you might as well just launch 10 executors (1GB) or run real standalone mode locally. I think it's better to fix the out-of-the-box case than to try to cover all potentially non-existent corner cases.
There was a problem hiding this comment.
Well, it would make local-cluster[] work like any other master, where you have to explicitly set the executor memory. I understand the desire to simplify things, but this is doing it at the cost of being inconsistent with other cluster managers.
(e.g. the same command line with a different master would behave differently - you'd fall back to having 1g of memory for executors instead of whatever was defined in the local-cluster string.)
There was a problem hiding this comment.
(Anyway, either way is probably fine, so go with your judgement. It just seems like a lot of code in SparkContext just to support that use case.)
There was a problem hiding this comment.
The inconsistency is already inherent with the parameters in local-cluster[], so I'm not introducing it here with this change. I personally think it's a really bad interface to force the user set executor memory in two different places and require that these two values match.
There was a problem hiding this comment.
also we're talking about a net addition of 7 LOC in SparkContext.scala, about half of which are comments and warning logs. It's really not that much code.
| // In other modes, use the configured memory if it exists | ||
| master match { | ||
| case SparkMasterRegex.LOCAL_CLUSTER_REGEX(_, _, em) => | ||
| if (configuredMemory.isDefined) { |
There was a problem hiding this comment.
Could you at least change this so that spark.executor.memory takes precedence if it's set? Then both use cases are possible. (Maybe someone is crazy enough to be trying dynamic allocation in local-cluster mode, or something else...)
|
Hi @andrewor14, is this still active? |
What changes were proposed in this pull request?
is supposed to launch 2 executors, each with 2GB of memory. However, when I ran this in master, I only get executors with 1GB memory. This patch fixes this problem.
How was this patch tested?
SparkSubmitSuite, manual tests.