You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by zuotingbing <gi...@git.apache.org> on 2018/10/26 09:42:52 UTC
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
GitHub user zuotingbing opened a pull request:
https://github.com/apache/spark/pull/22849
[SPARK-25852][Core] we should filter the workOffers of which freeCores>0 when make fake resource offers on all executors
## What changes were proposed in this pull request?
we should filter the workOffers of which freeCores>0 when make fake resource offers on all executors
## How was this patch tested?
Exist tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zuotingbing/spark SPARK-25852
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22849.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 #22849
----
commit 80f8c6e750c091299513b65c4f5fda5e6fd4bd22
Author: zuotingbing <zu...@...>
Date: 2018-10-26T09:39:30Z
[SPARK-25852][Core] we should filter the workOffers of which freeCores>0 when make fake resource offers on all executors
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers with...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22849
Yes, that is the comment I have been referring to. So it seems you can't filter, right? it's not scheduling work here. @jiangxb1987 do you know this part of the code?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers of w...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on the issue:
https://github.com/apache/spark/pull/22849
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers with...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/22849
What do you mean by "better performance" ? If that means we can spend less time on `TaskSchedulerImpl.resourceOffers()` then I agree it's true, but AFAIK it's never reported this can be a bottleneck of the whole cluster, so maybe the perf gain is trivial here. If you expect better task distribution over existing executors then I don't see any case can be improved by this proposed change. Please correct me if I'm wrong.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22849
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers of w...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22849
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers with...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/22849
It may happen that a busy executor is marked as lost and later it re-register to the driver, in that case currently we call `makeOffers()` and that will add the executor into `TaskSchedulerImpl.hostToExecutors`. This is bad implementation here since it shall not have depend on the `makeOffers()` function to update a unrelated protected val, but that's what we have for now.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers of w...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22849
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/22849#discussion_r229203395
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -240,7 +240,7 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
val taskDescs = CoarseGrainedSchedulerBackend.this.synchronized {
// Filter out executors under killing
val activeExecutors = executorDataMap.filterKeys(executorIsAlive)
- val workOffers = activeExecutors.map {
+ val workOffers = activeExecutors.filter(_._2.freeCores > 0).map {
--- End diff --
BTW, if freeCores < CPUS_PER_TASK, the code as fellow in resourceOffers() is inefficient since `o.cores / CPUS_PER_TASK` = 0
`val tasks = shuffledOffers.map(o => new ArrayBuffer[TaskDescription](o.cores / CPUS_PER_TASK))
val availableSlots = shuffledOffers.map(o => o.cores / CPUS_PER_TASK).sum`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22849#discussion_r228713414
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -240,7 +240,7 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
val taskDescs = CoarseGrainedSchedulerBackend.this.synchronized {
// Filter out executors under killing
val activeExecutors = executorDataMap.filterKeys(executorIsAlive)
- val workOffers = activeExecutors.map {
+ val workOffers = activeExecutors.filter(_._2.freeCores > 0).map {
--- End diff --
I don't know this code well but the comment above implies that it means to make an offer on all executors?
What is the performance impact anyway?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/22849#discussion_r228785046
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -240,7 +240,7 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
val taskDescs = CoarseGrainedSchedulerBackend.this.synchronized {
// Filter out executors under killing
val activeExecutors = executorDataMap.filterKeys(executorIsAlive)
- val workOffers = activeExecutors.map {
+ val workOffers = activeExecutors.filter(_._2.freeCores > 0).map {
--- End diff --
on our cluster there are many executors and tasksets/tasks. as we know there is a round-robin manner to fill each node with tasks which will be scheduled for each second("spark.scheduler.revive.interval", "1s"). it seams make no sense to schedule tasks to executors which have no free cores.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/22849#discussion_r229198651
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -240,7 +240,7 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
val taskDescs = CoarseGrainedSchedulerBackend.this.synchronized {
// Filter out executors under killing
val activeExecutors = executorDataMap.filterKeys(executorIsAlive)
- val workOffers = activeExecutors.map {
+ val workOffers = activeExecutors.filter(_._2.freeCores > 0).map {
--- End diff --
I checked the git log, at the beginning function `makeOffers` do not filter out any resource, maybe it is the reason why the comment is "Make fake resource offers on all executors"
`// Make fake resource offers on all executors
def makeOffers() {
launchTasks(scheduler.resourceOffers(
executorHost.toArray.map {case (id, host) => new WorkerOffer(id, host, freeCores(id))}))
}`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22849: [SPARK-25852][Core] we should filter the workOffe...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22849#discussion_r228936286
--- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala ---
@@ -240,7 +240,7 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
val taskDescs = CoarseGrainedSchedulerBackend.this.synchronized {
// Filter out executors under killing
val activeExecutors = executorDataMap.filterKeys(executorIsAlive)
- val workOffers = activeExecutors.map {
+ val workOffers = activeExecutors.filter(_._2.freeCores > 0).map {
--- End diff --
I'm saying there is a comment a few lines above that describes this as a fake offer that explicitly intends to contact all executors. I think we need to figure out if that's still relevant. I don't have git in front of me but check git blame or GitHub to see when this was written?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22849: [SPARK-25852][Core] we should filter the workOffers of w...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22849
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org