Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SPARK-1171: when executor is removed, we should minus totalCores instead of just freeCores on that executor #63

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
6 participants
Member

CodingCat commented Mar 3, 2014

https://spark-project.atlassian.net/browse/SPARK-1171

When the executor is removed, the current implementation will only minus the freeCores of that executor. Actually we should minus the totalCores...

Can one of the admins verify this patch?

Contributor

rxin commented Mar 3, 2014

Jenkins, add to whitelist.

Merged build triggered.

Merged build started.

Merged build finished.

One or more automated tests failed
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12964/

Merged build triggered.

Merged build started.

Merged build finished.

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12971/

@kayousterhout kayousterhout and 2 others commented on an outdated diff Mar 3, 2014

...in/scala/org/apache/spark/scheduler/WorkerOffer.scala
@@ -21,4 +21,6 @@ package org.apache.spark.scheduler
* Represents free resources available on an executor.
*/
private[spark]
-class WorkerOffer(val executorId: String, val host: String, val cores: Int)
+class WorkerOffer(val executorId: String, val host: String, var cores: Int) {
+ @transient val totalcores = cores
@kayousterhout

kayousterhout Mar 3, 2014

Contributor

Why does this need to be transient? also use camelcase for naming (totalCores)

@kayousterhout

kayousterhout Mar 3, 2014

Contributor

Actually on second thought, can CoarseGrainedSchedulerBackend just store the total cores for each worker in a hash map? I'd prefer that solution since other classes use WorkerOffer and don't use it to keep track of the total cores on each worker.

@markhamstra

markhamstra Mar 3, 2014

Contributor

+1 I'd also like to see WorkerOffer remain more like an immutable message type, with derived, mutable structures created only locally within the implementations that need it.

@andrewor14

andrewor14 Mar 3, 2014

Contributor

On that note, it seems to me that WorkerOffer should just be a case class, since all the constructor parameters are public vals anyway.

Merged build triggered.

Merged build started.

Merged build finished.

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12982/

Member

CodingCat commented Mar 4, 2014

@kayousterhout @markhamstra @andrewor14 Thank you for your comments,

I updated the code, how about this?

@markhamstra markhamstra commented on an outdated diff Mar 4, 2014

...scheduler/cluster/CoarseGrainedSchedulerBackend.scala
@@ -125,14 +126,17 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A
// Make fake resource offers on all executors
def makeOffers() {
- launchTasks(scheduler.resourceOffers(
- executorHost.toArray.map {case (id, host) => new WorkerOffer(id, host, freeCores(id))}))
+ // reconstruct workerOffers
+ workerOffers.foreach(o => workerOffers(o._1) =
+ new WorkerOffer(o._1, o._2.host, freeCores(o._1)))
@markhamstra

markhamstra Mar 4, 2014

Contributor

Now that WorkerOffer is a case class, you can do this and the one in makeOffers with the copy idiom:

workerOffers.keys.foreach { executorId => 
  workerOffers(executorId) = workerOffers(executorId).copy(cores = freeCores(executorId))
}

Merged build triggered.

Merged build started.

Merged build finished.

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12994/

@markhamstra markhamstra and 1 other commented on an outdated diff Mar 4, 2014

...in/scala/org/apache/spark/scheduler/WorkerOffer.scala
@@ -21,4 +21,4 @@ package org.apache.spark.scheduler
* Represents free resources available on an executor.
*/
private[spark]
-class WorkerOffer(val executorId: String, val host: String, val cores: Int)
+case class WorkerOffer(executorId: String, host: String, cores: Int);
@markhamstra

markhamstra Mar 4, 2014

Contributor

superfluous ';'

@CodingCat

CodingCat Mar 4, 2014

Member

oops, sorry, fixed

Merged build triggered.

Merged build started.

Contributor

kayousterhout commented Mar 4, 2014

This new version of the change doesn't look any simpler to me than the current version of the code and I think is a slightly confusing way of using worker offers to store info about the executors. Can you just remove executorAddress, the unused variable, and fix the bug, but keep the original way of generating worker offers?

Merged build finished.

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12995/

Merged build triggered.

Merged build started.

Merged build finished.

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12996/

Member

CodingCat commented Mar 5, 2014

How about this?

Contributor

kayousterhout commented Mar 5, 2014

This looks good -- I've merged this into master.

Member

CodingCat commented Mar 5, 2014

@kayousterhout Thank you very much!

@asfgit asfgit closed this in a3da508 Mar 5, 2014

@CodingCat CodingCat deleted the CodingCat:simplify_CoarseGrainedSchedulerBackend branch Mar 17, 2014

@jhartlaub jhartlaub referenced this pull request in jhartlaub/spark May 27, 2014

@mateiz @pwendell mateiz + pwendell Merge pull request #63 from pwendell/master
Fixing spark streaming example and a bug in examples build.

- Examples assembly included a log4j.properties which clobbered Spark's
- Example had an error where some classes weren't serializable
- Did some other clean-up in this example
(cherry picked from commit 28e9c2a)

Signed-off-by: Patrick Wendell <pwendell@gmail.com>
cee3b43

@CrazyJvm CrazyJvm added a commit to CrazyJvm/spark that referenced this pull request Jun 1, 2014

@CodingCat @CrazyJvm CodingCat + CrazyJvm SPARK-1171: when executor is removed, we should minus totalCores inst…
…ead of just freeCores on that executor


https://spark-project.atlassian.net/browse/SPARK-1171

When the executor is removed, the current implementation will only minus the freeCores of that executor. Actually we should minus the totalCores...

Author: CodingCat <zhunansjtu@gmail.com>
Author: Nan Zhu <CodingCat@users.noreply.github.com>

Closes #63 from CodingCat/simplify_CoarseGrainedSchedulerBackend and squashes the following commits:

f6bf93f [Nan Zhu] code clean
19c2bb4 [CodingCat] use copy idiom to reconstruct the workerOffers
43c13e9 [CodingCat] keep WorkerOffer immutable
af470d3 [CodingCat] style fix
0c0e409 [CodingCat] simplify the implementation of CoarseGrainedSchedulerBackend
d0b8fad

@gzm55 gzm55 added a commit to MediaV/spark that referenced this pull request Jul 17, 2014

@CodingCat @gzm55 CodingCat + gzm55 SPARK-1171: when executor is removed, we should minus totalCores inst…
…ead of just freeCores on that executor


https://spark-project.atlassian.net/browse/SPARK-1171

When the executor is removed, the current implementation will only minus the freeCores of that executor. Actually we should minus the totalCores...

Author: CodingCat <zhunansjtu@gmail.com>
Author: Nan Zhu <CodingCat@users.noreply.github.com>

Closes #63 from CodingCat/simplify_CoarseGrainedSchedulerBackend and squashes the following commits:

f6bf93f [Nan Zhu] code clean
19c2bb4 [CodingCat] use copy idiom to reconstruct the workerOffers
43c13e9 [CodingCat] keep WorkerOffer immutable
af470d3 [CodingCat] style fix
0c0e409 [CodingCat] simplify the implementation of CoarseGrainedSchedulerBackend
a9e41c7

@wli600 wli600 pushed a commit to wli600/spark that referenced this pull request Jul 29, 2015

@mbautin mbautin Merge pull request #63 from markhamstra/csd-1.4
Catch up with branch-1.4 bug fixes and bump jersey
25f3168

@JasonMWhite JasonMWhite pushed a commit to JasonMWhite/spark that referenced this pull request Dec 2, 2015

@kevincox kevincox Merge pull request #63 from Shopify/kevincox-spark-timestamp
"Fix" timestamp parsing and update spark.
f7b4551

@jlopezmalla jlopezmalla added a commit to mpenate/spark that referenced this pull request Sep 18, 2017

@pfcoperez @jlopezmalla pfcoperez + jlopezmalla Reduce method complexity in order to improve its readability & perfor…
…mance (#63)
9d7a92a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment