You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rdblue <gi...@git.apache.org> on 2016/05/26 22:43:58 UTC
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
GitHub user rdblue opened a pull request:
https://github.com/apache/spark/pull/13338
SPARK-13723: Change behavior of --num-executors with dynamic allocation.
## 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rdblue/spark SPARK-13723-num-executors-with-dynamic-allocation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/13338.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #13338
----
commit 5f6954943ed9278b3ebe6b871f20b14f66efd2f6
Author: Ryan Blue <bl...@apache.org>
Date: 2016-03-04T21:44:13Z
SPARK-13723: Change behavior of --num-executors with dynamic allocation.
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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66685722
--- Diff: docs/configuration.md ---
@@ -1158,6 +1158,10 @@ Apart from these, the following properties are also available, and may be useful
For more detail, see the description
<a href="job-scheduling.html#dynamic-resource-allocation">here</a>.
<br><br>
+ When this property is enabled, <code>spark.executor.instances</code> will affect the initial
+ number of executors, increasing the initial number if it is higher than the minimum or the
--- End diff --
I'll update docs after semantics are discussed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/13338#issuecomment-222033533
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59423/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/13338
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!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r65979763
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
+ }
+
+ /**
+ * Return the maximum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMaxExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.maxExecutors", Integer.MAX_VALUE)
+ }
+
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ math.max(
+ math.max(
+ conf.getInt("spark.dynamicAllocation.initialExecutors", 0),
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/13338
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r67038323
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2309,21 +2310,24 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ Seq(
+ conf.get(DYN_ALLOCATION_MIN_EXECUTORS),
+ conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS),
+ conf.get(EXECUTOR_INSTANCES).getOrElse(0)).max
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #60426 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60426/consoleFull)** for PR 13338 at commit [`47cff98`](https://github.com/apache/spark/commit/47cff98f765328c2219160ac61aec09573a85dbf).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/13338
Its in SparkSubmitArguments.scala function printUsageAndExit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66862738
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2309,21 +2310,24 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ Seq(
+ conf.get(DYN_ALLOCATION_MIN_EXECUTORS),
+ conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS),
+ conf.get(EXECUTOR_INSTANCES).getOrElse(0)).max
--- End diff --
Do we need to support environment variable `SPARK_EXECUTOR_INSTANCES`? Since it is not officially deprecated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #60427 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60427/consoleFull)** for PR 13338 at commit [`bf22b5a`](https://github.com/apache/spark/commit/bf22b5ab0bc8369949ac33833b078e7e13c7ce35).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66858439
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala ---
@@ -519,9 +519,9 @@ object YarnSparkHadoopUtil {
conf: SparkConf,
numExecutors: Int = DEFAULT_NUMBER_EXECUTORS): Int = {
if (Utils.isDynamicAllocationEnabled(conf)) {
- val minNumExecutors = conf.get(DYN_ALLOCATION_MIN_EXECUTORS)
- val initialNumExecutors = conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS)
- val maxNumExecutors = conf.get(DYN_ALLOCATION_MAX_EXECUTORS)
+ val minNumExecutors = Utils.getDynamicAllocationMinExecutors(conf)
--- End diff --
Sorry, I thought that DYN_ALLOCATION_MAX_EXECUTORS was a String. Marcelo's comment cleared it up and I've fixed these. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/13338#issuecomment-222016681
**[Test build #59423 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59423/consoleFull)** for PR 13338 at commit [`5f69549`](https://github.com/apache/spark/commit/5f6954943ed9278b3ebe6b871f20b14f66efd2f6).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/13338
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66858331
--- Diff: docs/configuration.md ---
@@ -1158,6 +1158,10 @@ Apart from these, the following properties are also available, and may be useful
For more detail, see the description
<a href="job-scheduling.html#dynamic-resource-allocation">here</a>.
<br><br>
+ When this property is enabled, <code>spark.executor.instances</code> will affect the initial
+ number of executors, increasing the initial number if it is higher than the minimum or the
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r64859655
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala ---
@@ -519,9 +519,9 @@ object YarnSparkHadoopUtil {
conf: SparkConf,
numExecutors: Int = DEFAULT_NUMBER_EXECUTORS): Int = {
if (Utils.isDynamicAllocationEnabled(conf)) {
- val minNumExecutors = conf.get(DYN_ALLOCATION_MIN_EXECUTORS)
- val initialNumExecutors = conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS)
- val maxNumExecutors = conf.get(DYN_ALLOCATION_MAX_EXECUTORS)
+ val minNumExecutors = Utils.getDynamicAllocationMinExecutors(conf)
--- End diff --
It would be better to use `DYN_ALLOCATION_MIN_EXECUTORS` from my understanding.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66684570
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala ---
@@ -519,9 +519,9 @@ object YarnSparkHadoopUtil {
conf: SparkConf,
numExecutors: Int = DEFAULT_NUMBER_EXECUTORS): Int = {
if (Utils.isDynamicAllocationEnabled(conf)) {
- val minNumExecutors = conf.get(DYN_ALLOCATION_MIN_EXECUTORS)
- val initialNumExecutors = conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS)
- val maxNumExecutors = conf.get(DYN_ALLOCATION_MAX_EXECUTORS)
+ val minNumExecutors = Utils.getDynamicAllocationMinExecutors(conf)
--- End diff --
Hmm, no. `sparkConf.get(DYN_ALLOCATION_MAX_EXECUTORS)` will return you an int, and if it's not defined in the conf, it will return you the default value, so you won't need to duplicate it in your code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r64859456
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
+ }
+
+ /**
+ * Return the maximum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMaxExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.maxExecutors", Integer.MAX_VALUE)
+ }
+
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ math.max(
+ math.max(
+ conf.getInt("spark.dynamicAllocation.initialExecutors", 0),
--- End diff --
Maybe we could change to who take precedence to who when both set, using `max` may not be a good choice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/13338#issuecomment-222033532
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60427/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r65978425
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
+ }
+
+ /**
+ * Return the maximum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMaxExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.maxExecutors", Integer.MAX_VALUE)
+ }
+
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ math.max(
+ math.max(
+ conf.getInt("spark.dynamicAllocation.initialExecutors", 0),
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-13723: Change behavior of --num-executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/13338#issuecomment-222033408
**[Test build #59423 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59423/consoleFull)** for PR 13338 at commit [`5f69549`](https://github.com/apache/spark/commit/5f6954943ed9278b3ebe6b871f20b14f66efd2f6).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66683769
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala ---
@@ -519,9 +519,9 @@ object YarnSparkHadoopUtil {
conf: SparkConf,
numExecutors: Int = DEFAULT_NUMBER_EXECUTORS): Int = {
if (Utils.isDynamicAllocationEnabled(conf)) {
- val minNumExecutors = conf.get(DYN_ALLOCATION_MIN_EXECUTORS)
- val initialNumExecutors = conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS)
- val maxNumExecutors = conf.get(DYN_ALLOCATION_MAX_EXECUTORS)
+ val minNumExecutors = Utils.getDynamicAllocationMinExecutors(conf)
--- End diff --
`DYN_ALLOCATION_MIN_EXECUTORS` is a String property. Instead of relying on the constants, this now standardizes on using functions in `Utils`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #61118 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61118/consoleFull)** for PR 13338 at commit [`0352883`](https://github.com/apache/spark/commit/0352883dba5aef4fd56d6aaddba129071b1d22ea).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/13338
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/13338
@tgravescs, where is the --help text for spark-submit? I'll update it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/13338
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r65974185
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
--- End diff --
as stated below we should use DYN_ALLOCATION_MIN_EXECUTORS, DYN_ALLOCATION_INITIAL_EXECUTORS, and DYN_ALLOCATION_MAX_EXECUTORS
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #60427 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60427/consoleFull)** for PR 13338 at commit [`bf22b5a`](https://github.com/apache/spark/commit/bf22b5ab0bc8369949ac33833b078e7e13c7ce35).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #60423 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60423/consoleFull)** for PR 13338 at commit [`b27a5b8`](https://github.com/apache/spark/commit/b27a5b85a12950e45dc8391808116104421543a7).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60426/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/13338
Thanks @tgravescs!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #61118 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61118/consoleFull)** for PR 13338 at commit [`0352883`](https://github.com/apache/spark/commit/0352883dba5aef4fd56d6aaddba129071b1d22ea).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60423/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on the issue:
https://github.com/apache/spark/pull/13338
Thanks for reviewing, everyone! I've made some comments and will update once we have consensus on util methods and semantics.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/13338
ping @rdblue
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/13338
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r65975449
--- Diff: docs/configuration.md ---
@@ -1158,6 +1158,10 @@ Apart from these, the following properties are also available, and may be useful
For more detail, see the description
<a href="job-scheduling.html#dynamic-resource-allocation">here</a>.
<br><br>
+ When this property is enabled, <code>spark.executor.instances</code> will affect the initial
+ number of executors, increasing the initial number if it is higher than the minimum or the
--- End diff --
I think the wording here is a bit confusing. Perhaps remove everything after the , and just say see spark.dynamicAllocation.initialExecutors and we can add more description there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66685637
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66793258
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
+ }
+
+ /**
+ * Return the maximum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMaxExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.maxExecutors", Integer.MAX_VALUE)
+ }
+
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ math.max(
+ math.max(
+ conf.getInt("spark.dynamicAllocation.initialExecutors", 0),
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/13338
I think we should also update the spark-submit --help for --num-executors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66858477
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66686584
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
+ }
+
+ /**
+ * Return the maximum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMaxExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.maxExecutors", Integer.MAX_VALUE)
+ }
+
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ math.max(
+ math.max(
+ conf.getInt("spark.dynamicAllocation.initialExecutors", 0),
--- End diff --
> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61118/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66973389
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2309,21 +2310,24 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ Seq(
+ conf.get(DYN_ALLOCATION_MIN_EXECUTORS),
+ conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS),
+ conf.get(EXECUTOR_INSTANCES).getOrElse(0)).max
--- End diff --
sorry I was mistaken it is documented on the yarn conf page.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/13338
+1, thanks @rdblue
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66685237
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
+ }
+
+ /**
+ * Return the maximum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMaxExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.maxExecutors", Integer.MAX_VALUE)
+ }
+
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ math.max(
+ math.max(
+ conf.getInt("spark.dynamicAllocation.initialExecutors", 0),
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66686443
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
--- End diff --
> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #60423 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60423/consoleFull)** for PR 13338 at commit [`b27a5b8`](https://github.com/apache/spark/commit/b27a5b85a12950e45dc8391808116104421543a7).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/13338
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r65985872
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by rdblue <gi...@git.apache.org>.
Github user rdblue commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66714041
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2262,21 +2262,39 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the minimum number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationMinExecutors(conf: SparkConf): Int = {
+ conf.getInt("spark.dynamicAllocation.minExecutors", 0)
--- End diff --
I see, so those aren't Strings. Thanks, I'll fix it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r67008417
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2309,21 +2310,24 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ Seq(
+ conf.get(DYN_ALLOCATION_MIN_EXECUTORS),
+ conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS),
+ conf.get(EXECUTOR_INSTANCES).getOrElse(0)).max
--- End diff --
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #13338: [SPARK-13723] [YARN] Change behavior of --num-exe...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/13338#discussion_r66972454
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2309,21 +2310,24 @@ private[spark] object Utils extends Logging {
}
/**
- * Return whether dynamic allocation is enabled in the given conf
- * Dynamic allocation and explicitly setting the number of executors are inherently
- * incompatible. In environments where dynamic allocation is turned on by default,
- * the latter should override the former (SPARK-9092).
+ * Return whether dynamic allocation is enabled in the given conf.
*/
def isDynamicAllocationEnabled(conf: SparkConf): Boolean = {
- val numExecutor = conf.getInt("spark.executor.instances", 0)
val dynamicAllocationEnabled = conf.getBoolean("spark.dynamicAllocation.enabled", false)
- if (numExecutor != 0 && dynamicAllocationEnabled) {
- logWarning("Dynamic Allocation and num executors both set, thus dynamic allocation disabled.")
- }
- numExecutor == 0 && dynamicAllocationEnabled &&
+ dynamicAllocationEnabled &&
(!isLocalMaster(conf) || conf.getBoolean("spark.dynamicAllocation.testing", false))
}
+ /**
+ * Return the initial number of executors for dynamic allocation.
+ */
+ def getDynamicAllocationInitialExecutors(conf: SparkConf): Int = {
+ Seq(
+ conf.get(DYN_ALLOCATION_MIN_EXECUTORS),
+ conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS),
+ conf.get(EXECUTOR_INSTANCES).getOrElse(0)).max
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #13338: [SPARK-13723] [YARN] Change behavior of --num-executors ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/13338
**[Test build #60426 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60426/consoleFull)** for PR 13338 at commit [`47cff98`](https://github.com/apache/spark/commit/47cff98f765328c2219160ac61aec09573a85dbf).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org