Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-1946] Submit tasks after (configured ratio) executors have been registered#900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
3f8c94137f7dc2e7b6272812c33c6cfb9ecce0868a42614540ecee9a4d6d847c6f052222ead121ac08b1b9f8326File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -46,9 +46,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) | ||
| 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) | ||
| // is equal to at least this value, that is double between 0 and 1. | ||
| var minRegisteredRatio = conf.getDouble("spark.scheduler.minRegisteredExecutorsRatio", 0) | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add the new configs to the user docs - see docs/configuration.md ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tgravescs Done. | ||
| if (minRegisteredRatio > 1) minRegisteredRatio = 1 | ||
| // Whatever minRegisteredExecutorsRatio is arrived, submit tasks after the time(milliseconds). | ||
| val maxRegisteredWaitingTime = | ||
| conf.getInt("spark.scheduler.maxRegisteredExecutorsWaitingTime", 30000) | ||
| val createTime = System.currentTimeMillis() | ||
| var ready = if (minRegisteredRatio <= 0) true else false | ||
| class DriverActor(sparkProperties: Seq[(String, String)]) extends Actor { | ||
| private val executorActor = new HashMap[String, ActorRef] | ||
| @@ -83,6 +93,12 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A | ||
| executorAddress(executorId) = sender.path.address | ||
| addressToExecutorId(sender.path.address) = executorId | ||
| totalCoreCount.addAndGet(cores) | ||
| if (executorActor.size >= totalExpectedExecutors.get() * minRegisteredRatio && !ready) { | ||
| ready = true | ||
| logInfo("SchedulerBackend is ready for scheduling beginning, registered executors: " + | ||
| executorActor.size + ", total expected executors: " + totalExpectedExecutors.get() + | ||
| ", minRegisteredExecutorsRatio: " + minRegisteredRatio) | ||
| } | ||
| makeOffers() | ||
| } | ||
| @@ -244,6 +260,19 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, actorSystem: A | ||
| throw new SparkException("Error notifying standalone scheduler's driver actor", e) | ||
| } | ||
| } | ||
| override def isReady(): Boolean = { | ||
| if (ready) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think saving the value of "ready" makes the code a bit difficult to read here, in part because it doesn't actually signal whether the backend is ready (since isReady() could return true even when ready is false). Can you just eliminate "ready" and move this line: if (executorActor.size >= totalExecutors.get() * minRegisteredRatio) { to here? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kayousterhout Now, the method is called per submitting tasks, it can return quickly by saving the value of "ready". Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my experience profiling the Spark scheduler, things like this do not affect performance in any significant way and in practice are often optimized out by JIT anyway, so we should opt for the more readable version Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be simpler to just inline the following code. There is no valid performance argument for separating it. Referencing the size of a HashMap in scala is a constant time operation. See https://github.com/scala/scala/blob/v2.10.4/src/library/scala/collection/mutable/HashTable.scala#L48
I don't see any performance argument for the current approach. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @pwendell@kayousterhout I am too thoughtful about these code's performance. ^_^ | ||
| return true | ||
| } | ||
| if ((System.currentTimeMillis() - createTime) >= maxRegisteredWaitingTime) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it might be nice to have a log statement here saying max time hit so we know when the scheduling began if debugging a job. | ||
| ready = true | ||
| logInfo("SchedulerBackend is ready for scheduling beginning after waiting " + | ||
| "maxRegisteredExecutorsWaitingTime: " + maxRegisteredWaitingTime) | ||
| return true | ||
| } | ||
| false | ||
| } | ||
| } | ||
| private[spark] object CoarseGrainedSchedulerBackend { | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.spark.scheduler.cluster | ||
| import org.apache.spark.SparkContext | ||
| import org.apache.spark.deploy.yarn.ApplicationMasterArguments | ||
| import org.apache.spark.scheduler.TaskSchedulerImpl | ||
| import org.apache.spark.util.IntParam | ||
| private[spark] class YarnClusterSchedulerBackend( | ||
| scheduler: TaskSchedulerImpl, | ||
| sc: SparkContext) | ||
| extends CoarseGrainedSchedulerBackend(scheduler, sc.env.actorSystem) { | ||
| override def start() { | ||
| super.start() | ||
| var numExecutors = ApplicationMasterArguments.DEFAULT_NUMBER_EXECUTORS | ||
| if (System.getenv("SPARK_EXECUTOR_INSTANCES") != null) { | ||
| numExecutors = IntParam.unapply(System.getenv("SPARK_EXECUTOR_INSTANCES")).getOrElse(numExecutors) | ||
| } | ||
| // System property can override environment variable. | ||
| numExecutors = sc.getConf.getInt("spark.executor.instances", numExecutors) | ||
| totalExpectedExecutors.set(numExecutors) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -164,6 +164,7 @@ class ApplicationMaster(args: ApplicationMasterArguments, conf: Configuration, | ||
| private def startUserClass(): Thread = { | ||
| logInfo("Starting the user JAR in a separate Thread") | ||
| System.setProperty("spark.executor.instances", args.numExecutors.toString) | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need to set this here? Is this for the case when args.numExecutors was set by SPARK_EXECUTOR_INSTANCES (since otherwise it seems like spark.executor.instances will already be set, right)? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kayousterhout It's for yarn-cluster mode. | ||
| val mainMethod = Class.forName( | ||
| args.userClass, | ||
| false, | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a check for > 1 and set to 1 if over. I initially set it to 40 in a test thinking that meant 40%. I guess the documentation will also clarify.