You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by li-zhihui <gi...@git.apache.org> on 2014/07/22 11:05:00 UTC
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
GitHub user li-zhihui opened a pull request:
https://github.com/apache/spark/pull/1525
Fix race condition at SchedulerBackend.isReady in standalone mode
In SPARK-1946(PR #900), configuration <code>spark.scheduler.minRegisteredExecutorsRatio</code> was introduced. However, in standalone mode, there is a race condition where isReady() can return true because totalExpectedExecutors has not been correctly set.
Because expected executors is uncertain in standalone mode, the PR try to use CPU cores(<code>--total-executor-cores</code>) as expected resources to judge whether SchedulerBackend is ready.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/li-zhihui/spark fixre4s
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1525.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 #1525
----
commit 8b54316c77d086ea3454419ebba92003707bbd76
Author: li-zhihui <zh...@intel.com>
Date: 2014-07-22T08:15:40Z
Fix race condition at SchedulerBackend.isReady in standalone mode
----
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49825514
Jenkins, test this please
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49714878
@kayousterhout @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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49956131
@kayousterhout I guess your meaning is "totalExpectedResources" (no variable "totalRegisteredResources").
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15242315
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/SparkDeploySchedulerBackend.scala ---
@@ -108,4 +108,8 @@ private[spark] class SparkDeploySchedulerBackend(
logInfo("Executor %s removed: %s".format(fullId, message))
removeExecutor(fullId.split("/")(1), reason.toString)
}
+
+ override def checkRegisteredResources(): Boolean = {
--- End diff --
or `sufficientResourcesRegistered`
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51436526
Jenkins, test this please.
---
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: Fix race condition at SchedulerBackend.isReady...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15268935
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ // Whatever minRegisteredRatio is arrived, submit tasks after the time(milliseconds).
--- End diff --
Ah, I see -- sorry. Looks like this is what we want? `// Submit tasks after maxRegisteredWaitingTime milliseconds if minRegisteredRatio has not yet been reached`
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15725931
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
- if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ var minRegisteredRatio =
+ math.min(1, conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0))
+ // Submit tasks after maxRegisteredWaitingTime milliseconds
+ // if minRegisteredRatio has not yet been reached
val maxRegisteredWaitingTime =
- conf.getInt("spark.scheduler.maxRegisteredExecutorsWaitingTime", 30000)
+ conf.getInt("spark.scheduler.maxRegisteredResourcesWaitingTime", 30000)
--- End diff --
I think this was introduced in 1.1 ? If yes, ok to rename
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49960668
@tgravescs @kayousterhout I add a new commit.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50850584
@tgravescs @kayousterhout can you close this PR before code frozen of 1.1 release? Otherwise, it would result in incompatible configuration property name because the PR rename <code>spark.scheduler.maxRegisteredExecutorsWaitingTime</code> to <code>spark.scheduler.maxRegisteredResourcesWaitingTime</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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49825479
can you please also file a jira for this
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15325405
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/SparkDeploySchedulerBackend.scala ---
@@ -36,6 +36,7 @@ private[spark] class SparkDeploySchedulerBackend(
var shutdownCallback : (SparkDeploySchedulerBackend) => Unit = _
val maxCores = conf.getOption("spark.cores.max").map(_.toInt)
+ totalExpectedResources.getAndSet(maxCores.getOrElse(0))
--- End diff --
oops, thanks @kayousterhout
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49957064
@kayousterhout I think use totalExpectedCores and totalExpectedExecutors replace totalExpectedResources is a good idea, 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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15725840
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
--- End diff --
one more nit: can we call this totalRegisteredExecutors?
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15308341
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/SparkDeploySchedulerBackend.scala ---
@@ -36,6 +36,7 @@ private[spark] class SparkDeploySchedulerBackend(
var shutdownCallback : (SparkDeploySchedulerBackend) => Unit = _
val maxCores = conf.getOption("spark.cores.max").map(_.toInt)
+ totalExpectedResources.getAndSet(maxCores.getOrElse(0))
--- End diff --
Why "getAndSet" here instead of just "set"?
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49830484
QA results for PR 1525:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17006/consoleFull
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15272948
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -268,14 +264,18 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
}
}
+ def sufficientResourcesRegistered(): Boolean = true
+
override def isReady(): Boolean = {
- if (ready) {
+ if (sufficientResourcesRegistered) {
+ logInfo("SchedulerBackend is ready for scheduling beginning" +
+ ", total expected resources: " + totalExpectedResources.get() +
+ ", minRegisteredResourcesRatio: " + minRegisteredRatio)
return true
}
if ((System.currentTimeMillis() - createTime) >= maxRegisteredWaitingTime) {
- ready = true
logInfo("SchedulerBackend is ready for scheduling beginning after waiting " +
- "maxRegisteredExecutorsWaitingTime: " + maxRegisteredWaitingTime)
+ "maxRegisteredResourcesWaitingTime(ms): " + maxRegisteredWaitingTime)
--- End diff --
@markhamstra 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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15268342
--- Diff: docs/configuration.md ---
@@ -707,21 +707,22 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
</tr>
- <td><code>spark.scheduler.minRegisteredExecutorsRatio</code></td>
+ <td><code>spark.scheduler.minRegisteredResourcesRatio</code></td>
<td>0</td>
<td>
- The minimum ratio of registered executors (registered executors / total expected executors)
+ The minimum ratio of registered resources (registered resources / total expected resources)
+ (resources are executors in yarn mode, CPU cores in standalone and mesos mode)
--- End diff --
nit, but mesos isn't covered yet.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50855070
I will take a look at this tomorrow.
On Thu, Jul 31, 2014 at 10:37 PM, Zhihui Li <no...@github.com>
wrote:
> @tgravescs <https://github.com/tgravescs> @kayousterhout
> <https://github.com/kayousterhout> can you close this PR before code
> frozen of 1.1 release? Otherwise, it would result in incompatible
> configuration property name because the PR rename
> spark.scheduler.maxRegisteredExecutorsWaitingTime to
> spark.scheduler.maxRegisteredResourcesWaitingTime
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/1525#issuecomment-50850584>.
>
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49836422
I add a new commit, @tgravescs @markhamstra
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49874814
QA tests have started for PR 1525. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17033/consoleFull
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15268932
--- Diff: docs/configuration.md ---
@@ -707,21 +707,22 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
</tr>
- <td><code>spark.scheduler.minRegisteredExecutorsRatio</code></td>
+ <td><code>spark.scheduler.minRegisteredResourcesRatio</code></td>
<td>0</td>
<td>
- The minimum ratio of registered executors (registered executors / total expected executors)
+ The minimum ratio of registered resources (registered resources / total expected resources)
+ (resources are executors in yarn mode, CPU cores in standalone and mesos mode)
--- End diff --
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50954397
My preference is also to remove this for standalone mode (as mentioned in the original PR, #900) -- but adding @tgravescs who looked quite a bit at the original PR to see if we're forgetting something important here!
---
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-2635] Fix race condition at SchedulerBa...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15272634
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -268,14 +264,18 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
}
}
+ def sufficientResourcesRegistered(): Boolean = true
+
override def isReady(): Boolean = {
- if (ready) {
+ if (sufficientResourcesRegistered) {
+ logInfo("SchedulerBackend is ready for scheduling beginning" +
+ ", total expected resources: " + totalExpectedResources.get() +
+ ", minRegisteredResourcesRatio: " + minRegisteredRatio)
return true
}
if ((System.currentTimeMillis() - createTime) >= maxRegisteredWaitingTime) {
- ready = true
logInfo("SchedulerBackend is ready for scheduling beginning after waiting " +
- "maxRegisteredExecutorsWaitingTime: " + maxRegisteredWaitingTime)
+ "maxRegisteredResourcesWaitingTime(ms): " + maxRegisteredWaitingTime)
--- End diff --
I'd do these two log messages with string interpolation instead of using `+`. http://docs.scala-lang.org/overviews/core/string-interpolation.html
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49825654
QA tests have started for PR 1525. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17006/consoleFull
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15268755
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/SparkDeploySchedulerBackend.scala ---
@@ -108,4 +108,8 @@ private[spark] class SparkDeploySchedulerBackend(
logInfo("Executor %s removed: %s".format(fullId, message))
removeExecutor(fullId.split("/")(1), reason.toString)
}
+
+ override def checkRegisteredResources(): Boolean = {
--- End diff --
good, thanks @markhamstra
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50950169
Hey all @kayousterhout asked me to look at this. It doesn't make semantic sense to expose `spark.scheduler.minRegisteredExecutorsRatio` in standalone mode because applications in standalone mode to not request a fixed number of executors a-priori. So my proposal is to remove this feature in standalone mode alltogether. The semantics of `spark.cores.max` is just the maximum number. In some cases users run jobs with this set well above the number of available cores (because they decided to run on a smaller cluster) and it is fully supported. If we enforce a minimum, it will cause all jobs to hang for those users.
If users in standalone mode want to wait, for now they should add their own code to sleep until the number of desired executors appears. They can do this by calling `sc.getExecutorStorageStatus.size()` or we can also add an API called `sc.getNumExecutors` that does this.
Also curious what @mateiz and @aarondav think about this. I haven't been following this patch previously.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by tnachen <gi...@git.apache.org>.
Github user tnachen commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50972582
If what @pwendell said is true about sparks.core.max then I think probably need to rethink how to approach this regardless what mode we're in. I think I gears towards safety first since this is just an optimization.
I'm still getting up to speed about the scheduling in spark, but I believe the locality calculation is shared in TaskScheduler 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: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49921298
I find it a bit confusing that "totalRegisteredResources" can refer to cores (in standalone mode) or executors (in Yarn mode). Can we just use different variables in each of the two cases and name them appropriately (so totalRegisteredCores and totalRegisteredExecutors)?
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49874493
Jenkins, test this please
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15270909
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
--- End diff --
Sorry, was doing something dumb. Leave it a `var` but clean up the initialization.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51639499
Changes look good to me.
---
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-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15738382
--- Diff: yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClusterSchedulerBackend.scala ---
@@ -40,6 +41,10 @@ private[spark] class YarnClusterSchedulerBackend(
}
// System property can override environment variable.
numExecutors = sc.getConf.getInt("spark.executor.instances", numExecutors)
- totalExpectedExecutors.set(numExecutors)
+ totalExpectedExecutors = numExecutors
--- End diff --
@kayousterhout done
---
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-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15271057
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
--- End diff --
Thanks @markhamstra the var member also is used in https://github.com/apache/spark/pull/1525/files#diff-ae6a41a938a767e5bb97b5d738371a5bR34
math.min is a good way.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15738384
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
--- End diff --
@kayousterhout done
---
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-2635] Fix race condition at SchedulerBa...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15725875
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
- if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ var minRegisteredRatio =
+ math.min(1, conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0))
+ // Submit tasks after maxRegisteredWaitingTime milliseconds
+ // if minRegisteredRatio has not yet been reached
val maxRegisteredWaitingTime =
- conf.getInt("spark.scheduler.maxRegisteredExecutorsWaitingTime", 30000)
+ conf.getInt("spark.scheduler.maxRegisteredResourcesWaitingTime", 30000)
--- End diff --
interface change ?
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15270593
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
--- End diff --
Looks like `minRegisteredRatio` is only needed within this class and doesn't need to be a var:
```scala
private val minRegisteredRatio = math.min(1, conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0))
```
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49873884
Jenkins, this is okay to test
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51440477
QA results for PR 1525:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18109/consoleFull
---
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-2635] Fix race condition at SchedulerBa...
Posted by tnachen <gi...@git.apache.org>.
Github user tnachen commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51011745
I see, the max waiting time does mitigate this. I think minimally probably worth commenting on the configuration to warn the potential hanging and the max timeout will not allow the scheduler to wait forever.
---
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-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15269102
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ // Whatever minRegisteredRatio is arrived, submit tasks after the time(milliseconds).
--- End diff --
good, thanks @markhamstra
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50962993
Maybe we should think the feature in standalone mode and mesos mode together.
Is it necessary in mesos mode? https://github.com/apache/spark/pull/1462
@tnachen
---
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-2635] Fix race condition at SchedulerBa...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51011136
As @pwendell says, the configuration is disable in standalone mode. And in the worst situation, it sleep <code>spark.scheduler.maxRegisteredResourcesWaitingTime</code> (just as currently recommendation). So, my opinion is to keep the configuration in all deploy mode(yarn, standalone, mesos).
---
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: Fix race condition at SchedulerBackend.isReady...
Posted by li-zhihui <gi...@git.apache.org>.
Github user li-zhihui commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15268735
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ // Whatever minRegisteredRatio is arrived, submit tasks after the time(milliseconds).
--- End diff --
Thanks @markhamstra , but I think the code means that submit tasks time if minRegisteredRatio is not reached.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51531440
I think the verdict here is to leave this feature in, and this patch looks
good to me if it looks good to you, Tom.
On Thu, Aug 7, 2014 at 1:54 PM, Tom Graves <no...@github.com> wrote:
> We should figure out before we release 1.1 which we are going to support
> this on and make sure the config name is good. I'm fine either way as long
> as it works on the yarn side.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/1525#issuecomment-51530339>.
>
---
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: Fix race condition at SchedulerBackend.isReady...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49714817
Can one of the admins verify this patch?
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15725825
--- Diff: yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClusterSchedulerBackend.scala ---
@@ -40,6 +41,10 @@ private[spark] class YarnClusterSchedulerBackend(
}
// System property can override environment variable.
numExecutors = sc.getConf.getInt("spark.executor.instances", numExecutors)
- totalExpectedExecutors.set(numExecutors)
+ totalExpectedExecutors = numExecutors
--- End diff --
nit: make this one line with the one above
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15242266
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/SparkDeploySchedulerBackend.scala ---
@@ -108,4 +108,8 @@ private[spark] class SparkDeploySchedulerBackend(
logInfo("Executor %s removed: %s".format(fullId, message))
removeExecutor(fullId.split("/")(1), reason.toString)
}
+
+ override def checkRegisteredResources(): Boolean = {
--- End diff --
I'd prefer the name to indicate what condition is being checked, so something like `sufficientRegisteredResources`.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r16005054
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
- if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ var minRegisteredRatio =
+ math.min(1, conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0))
+ // Submit tasks after maxRegisteredWaitingTime milliseconds
+ // if minRegisteredRatio has not yet been reached
val maxRegisteredWaitingTime =
- conf.getInt("spark.scheduler.maxRegisteredExecutorsWaitingTime", 30000)
+ conf.getInt("spark.scheduler.maxRegisteredResourcesWaitingTime", 30000)
--- End diff --
yes it was introduced in 1.1
---
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-2635] Fix race condition at SchedulerBa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-49889752
QA results for PR 1525:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17033/consoleFull
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15270679
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
--- End diff --
...actually, it doesn't look like it's used at all anymore except in a log message.
---
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.
---
[GitHub] spark pull request: Fix race condition at SchedulerBackend.isReady...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1525#discussion_r15240513
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -47,19 +47,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
{
// Use an atomic variable to track total number of cores in the cluster for simplicity and speed
var totalCoreCount = new AtomicInteger(0)
- var totalExpectedExecutors = new AtomicInteger(0)
+ var totalExecutors = new AtomicInteger(0)
+ var totalExpectedResources = new AtomicInteger(0)
val conf = scheduler.sc.conf
private val timeout = AkkaUtils.askTimeout(conf)
private val akkaFrameSize = AkkaUtils.maxFrameSizeBytes(conf)
- // Submit tasks only after (registered executors / total expected executors)
+ // Submit tasks only after (registered resources / total expected resources)
// is equal to at least this value, that is double between 0 and 1.
- var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0)
+ var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0)
if (minRegisteredRatio > 1) minRegisteredRatio = 1
- // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds).
+ // Whatever minRegisteredRatio is arrived, submit tasks after the time(milliseconds).
--- End diff --
// Submit tasks time(milliseconds) after minRegisteredRatio is reached
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51530339
We should figure out before we release 1.1 which we are going to support this on and make sure the config name is good. I'm fine either way as long as it works on the yarn side.
---
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-2635] Fix race condition at SchedulerBa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51436721
QA tests have started for PR 1525. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18109/consoleFull
---
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-2635] Fix race condition at SchedulerBa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1525
---
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-2635] Fix race condition at SchedulerBa...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-50950223
I guess this currently is disabled by default, so that's good (it won't break behavior) but I still don't think `spark.cores.max` is really meant to be used in this way.
---
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.
---
[GitHub] spark pull request: [SPARK-2635] Fix race condition at SchedulerBa...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1525#issuecomment-51678146
Okay cool LGTM too... after some thought I'm okay to leave it in. The name right now is a bit awkward, but I don't see a better option at this point. Thanks @kayousterhout and @tgravescs for looking at this.
---
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