[SPARK-13723] [YARN] Change behavior of --num-executors with dynamic allocation. by rdblue · Pull Request #13338 · apache/spark

rdblue · 2016-05-26T22:43:57Z

What changes were proposed in this pull request?

This changes the behavior of --num-executors and spark.executor.instances when using dynamic allocation. Instead of turning dynamic allocation off, it uses the value for the initial number of executors.

This changes was discussed on SPARK-13723. I highly recommend using it while we can change the behavior for 2.0.0. In practice, the 1.x behavior causes unexpected behavior for users (it is not clear that it disables dynamic allocation) and wastes cluster resources because users rarely notice the log message.

How was this patch tested?

This patch updates tests and adds a test for Utils.getDynamicAllocationInitialExecutors.

SparkQA · 2016-05-27T00:45:49Z

Test build #59423 has finished for PR 13338 at commit 5f69549.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jerryshao · 2016-05-27T05:37:11Z

core/src/main/scala/org/apache/spark/util/Utils.scala

Maybe we could change to who take precedence to who when both set, using max may not be a good choice.

I'm not overly concerned with this as long as its clear to the users which way we chose. It feels like the user would use --num-executors more easily or accidentally then specifying spark.dynamicAllocation.initialExecutors.

How about leaving it max and just print a warning if we find both set and tell them what we choose.

I'm also not sure there is a good way to do this right now with the ConfigEntry/Builder stuff thouh. @vanzin

I don't think the config API can handle both calls to math.max here... you could replace the inner one with a fallback config (initialExecutors falls back to the value of spark.executor.instances if not set), although even then the semantics are slightly different.

In any case, this might be more readable as:

Seq( conf.getInt("spark.dynamicAllocation.initialExecutors", 0), conf.getInt("spark.executor.instances", 0), getDynamicAllocationMinExecutors(conf)).max

Assuming I got the semantics right.

I'm not sure what you mean by the config API can't handle both calls to max.

I'm happy to make the change for this to be more readable, but I think we should decide on semantics first. I think a reasonable way to handle this is to use the max of all 3, as implemented. The min value should clearly be handled this way -- starting at initial less than min causes an immediate change to the min -- so the question is how to handle both spark.executor.instances and spark.dynamicAllocation.initialExecutors.

I think it's reasonable to use either one in different situations so I don't think it makes much sense to complain if they're both set. My job could have initialExecutors set in its config, but I can run it with --num-executors to bump up the value. Given that use case, I don't think it would be a good idea to have initialExecutors override spark.executor.instances.

That leaves whether spark.executor.instances should override initialExecutors or whether the initial number should be the max of the two. I don't have a strong opinion here, but I opted for the max of the two values.

I'm not sure what you mean by the config API can't handle both calls to max.

I was replying to Tom's question, you can ignore that part.

I'm fine with max and as I mentioned I agree that I think users might use or accidentally use --num-executors to bump up the value. Although I'd rather not encourage that behavior because its intermixing these things and --num-executors is ambiguous on the behavior (based on the dynamic allocation on or off), which is why I suggested the warning. The warning was also in case someone specified --num-executors and thought they were getting static number but instead had dynamic allocation on so its initial.

I would also be fine with just an info message here printing what allocation method we are using and the number chosen.

tgravescs · 2016-06-06T21:38:25Z

core/src/main/scala/org/apache/spark/util/Utils.scala

as stated below we should use DYN_ALLOCATION_MIN_EXECUTORS, DYN_ALLOCATION_INITIAL_EXECUTORS, and DYN_ALLOCATION_MAX_EXECUTORS

yep, don't add these methods. instead:

import org.apache.spark.internal.config._ conf.get(DYN_ALLOCATION_MIN_EXECUTORS)

Also recommend going through your change and modifying all the places that hardcode the config key to use the constants.

What's the value of these constants over util methods? These constants live in the yarn module, but there are uses in the ExecutorAllocationManager in core. Rather than having the constants in two places, with possibly different defaults, I thought a util method was appropriate.

What's the value of these constants over util methods?

They encapsulate key, type and default value already, so you'd avoid duplicating those in your code.

These constants live in the yarn module

No, they're in core/src/main/scala/org/apache/spark/internal/config/package.scala.

Rather than having the constants in two places, with possibly different defaults, I thought a util method was appropriate.

That's exactly what those constants are.

I see, so those aren't Strings. Thanks, I'll fix it.

tgravescs · 2016-06-06T21:50:49Z

I think we should also update the spark-submit --help for --num-executors.

rdblue · 2016-06-10T21:48:52Z

Thanks for reviewing, everyone! I've made some comments and will update once we have consensus on util methods and semantics.

rdblue · 2016-06-13T20:12:45Z

@tgravescs, where is the --help text for spark-submit? I'll update it.

rdblue · 2016-06-13T20:13:46Z

I just pushed a version that addresses the comments so far (other than the spark-submit help text). Thanks for looking at this, @vanzin, @jerryshao, and @tgravescs!

SparkQA · 2016-06-13T20:16:41Z

Test build #60423 has finished for PR 13338 at commit b27a5b8.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-06-13T20:27:19Z

Its in SparkSubmitArguments.scala function printUsageAndExit

Previously, using --num-executors or spark.executor.instances with dynamic allocation enabled would disable dynamic allocation. This updates the behavior so these settings change the initial number of executors and do not disable dynamic allocation.

jerryshao · 2016-06-13T20:37:51Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+    Seq(
+      conf.get(DYN_ALLOCATION_MIN_EXECUTORS),
+      conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS),
+      conf.get(EXECUTOR_INSTANCES).getOrElse(0)).max


Do we need to support environment variable SPARK_EXECUTOR_INSTANCES? Since it is not officially deprecated.

EXECUTOR_INSTANCES isn't deprecated. This is a conf (spark.executor.instances), its not documented but I'm pretty sure I've seen people using it instead of --num-executors and that is what SparkSubmit uses.

sorry I was mistaken it is documented on the yarn conf page.

I think what I mean is env variable SPARK_EXECUTOR_INSTANCES, we should also consider this variable when getting dynamic allocation initial executors (in case someone is using this variable).

oh I see. Well we didn't support it before in this routine and I would prefer not to add more support to env variables as I would rather see them all removed. So I would say no, at least not under this jira. The only place I see it being used is in the YarnSparkHadoopUtil. getInitialTargetExecutorNumber so I would say we file a separate jira to remove it and clean up comments in code in other places.

SparkQA · 2016-06-13T22:15:50Z

Test build #60427 has finished for PR 13338 at commit bf22b5a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-06-13T22:38:16Z

Test build #60426 has finished for PR 13338 at commit 47cff98.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-06-14T13:55:33Z

we need to update running-on-yarn.md the description for spark.executor.instances as it says dynamic allocation will be turned off when its specified.

otherwise LGTM.

tgravescs · 2016-06-23T13:54:01Z

ping @rdblue

tgravescs · 2016-06-23T13:55:13Z

I really want to get this into the 2.0 release since its a change in behavior if you don't have time I'll put up pr with minor changes.

rdblue · 2016-06-23T16:25:42Z

@tgravescs, I've updated it. Sorry about the delay, for some reason the notifications for this issue didn't make it to my inbox so I wasn't seeing updates.

SparkQA · 2016-06-23T18:59:46Z

Test build #61118 has finished for PR 13338 at commit 0352883.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-06-23T19:02:47Z

+1, thanks @rdblue

…llocation. ## What changes were proposed in this pull request? This changes the behavior of --num-executors and spark.executor.instances when using dynamic allocation. Instead of turning dynamic allocation off, it uses the value for the initial number of executors. This changes was discussed on [SPARK-13723](https://issues.apache.org/jira/browse/SPARK-13723). I highly recommend using it while we can change the behavior for 2.0.0. In practice, the 1.x behavior causes unexpected behavior for users (it is not clear that it disables dynamic allocation) and wastes cluster resources because users rarely notice the log message. ## How was this patch tested? This patch updates tests and adds a test for Utils.getDynamicAllocationInitialExecutors. Author: Ryan Blue <blue@apache.org> Closes #13338 from rdblue/SPARK-13723-num-executors-with-dynamic-allocation. (cherry picked from commit 738f134) Signed-off-by: Tom Graves <tgraves@yahoo-inc.com>

rdblue · 2016-06-23T19:47:39Z

Thanks @tgravescs!

jerryshao reviewed May 27, 2016
View reviewed changes

rdblue changed the title ~~SPARK-13723: Change behavior of --num-executors with dynamic allocation.~~ [SPARK-13723] [YARN] Change behavior of --num-executors with dynamic allocation. Jun 2, 2016

tgravescs reviewed Jun 6, 2016
View reviewed changes

rdblue force-pushed the SPARK-13723-num-executors-with-dynamic-allocation branch from 5f69549 to b27a5b8 Compare June 13, 2016 20:11

rdblue force-pushed the SPARK-13723-num-executors-with-dynamic-allocation branch from b27a5b8 to 47cff98 Compare June 13, 2016 20:29

SPARK-13723: Update --num-executors in spark-submit help.

bf22b5a

jerryshao reviewed Jun 13, 2016
View reviewed changes

SPARK-13723: Update running-on-yarn.md for these changes.

0352883

asfgit closed this in 738f134 Jun 23, 2016

Conversation

rdblue commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs commented Jun 6, 2016

Uh oh!

rdblue commented Jun 10, 2016

Uh oh!

rdblue commented Jun 13, 2016

Uh oh!

rdblue commented Jun 13, 2016

Uh oh!

SparkQA commented Jun 13, 2016

Uh oh!

tgravescs commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 13, 2016

Uh oh!

SparkQA commented Jun 13, 2016

Uh oh!

tgravescs commented Jun 14, 2016

Uh oh!

tgravescs commented Jun 23, 2016

Uh oh!

tgravescs commented Jun 23, 2016

Uh oh!

rdblue commented Jun 23, 2016

Uh oh!

SparkQA commented Jun 23, 2016

Uh oh!

tgravescs commented Jun 23, 2016

Uh oh!

rdblue commented Jun 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rdblue commented May 26, 2016 •

edited

Loading