Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,8 @@ private[spark] class TaskSchedulerImpl(
// By default, rack is unknown
def getRackForHost(value: String): Option[String] = None
Comment thread
LantaoJin marked this conversation as resolved.

def getRacksForHosts(values: List[String]): List[String] = Nil
Comment thread
LantaoJin marked this conversation as resolved.
Outdated

private def waitBackendReady(): Unit = {
if (backend.isReady) {
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,23 @@ private[spark] class TaskSetManager(
t.epoch = epoch
}

// An array to store preferred location and its task index
private val locationWithTaskIndex: ArrayBuffer[(String, Int)] = new ArrayBuffer[(String, Int)]()
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
private val addTaskStartTime = System.nanoTime()
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
// Add all our tasks to the pending lists. We do this in reverse order
// of task index so that tasks with low indices get launched first.
for (i <- (0 until numTasks).reverse) {
addPendingTask(i)
addPendingTask(i, true)
}
// Convert preferred location list to rack list in one invocation and zip with the origin index
private val rackWithTaskIndex = sched.getRacksForHosts(locationWithTaskIndex.map(_._1).toList)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be worth doing some de-duping here? It would not be unusual to have a taskset with 10K tasks on 100 hosts. I see that CachedDNSToSwitchMapping will cache the lookup itself, but doesn't seem necessary to create those intermediate data structures with 10K elements.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I meant something like this for avoiding leaving around locationWithTaskIndex and doing the deduping.
squito@fc0b308

Feel free to pull in that commit to your change if you think its helpful.

The de-duping thing is minor, but I am concerned that the locationWithTaskIndex variable is going to be confusing if its left around as a private member variable, even though its only meaningful in this limited context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be worth doing some de-duping here? It would not be unusual to have a taskset with 10K tasks on 100 hosts. I see that CachedDNSToSwitchMapping will cache the lookup itself, but doesn't seem necessary to create those intermediate data structures with 10K elements.

We can not de-duping this List, or the logic about choosing a task to launch will incorrect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The de-duping thing is minor, but I am concerned that the locationWithTaskIndex variable is going to be confusing if its left around as a private member variable, even though its only meaningful in this limited context.

Yes, I don't want to do any de-duping here. I will refactor this part for getting more readable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On de-duping -- I know you can't do it blindly, as then you won't correctly keep track of which task goes with which host. But if you look at my proposed change, I think it keeps track of the correct mapping back to the original task indices (it certainly should be possible, anyway, even if that exact version doesn't work).

Looking at CachedDNSToSwitchMapping, if you repeat the same host 1000 times, if that host is not in the cache yet, it will repeat that lookup 1000 times. getUncachedHosts does a lookup to check if its in the cache, but if not it'll just pass each copy on to the uncachedHosts variable. Then that just gets passed through to rawMapping.resolve(). (Perhaps getUncachedHosts really should return a Set<String>.)

so after looking at that code, I'm more convinced de-duping would help you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor with the de-duping approach.

.zip(locationWithTaskIndex.map(_._2))
for ((rack, index) <- rackWithTaskIndex) {
pendingTasksForRack.getOrElseUpdate(rack, new ArrayBuffer) += index
}
// visible for testing
private[scheduler] val addTaskElapsedTime = (System.nanoTime() - addTaskStartTime) / 1e9
logInfo(s"Adding pending task takes $addTaskElapsedTime seconds")

/**
* Track the set of locality levels which are valid given the tasks locality preferences and
Expand All @@ -214,7 +226,7 @@ private[spark] class TaskSetManager(
private[scheduler] var emittedTaskSizeWarning = false

/** Add a task to all the pending-task lists that it should be on. */
private[spark] def addPendingTask(index: Int) {
private[spark] def addPendingTask(index: Int, initialing: Boolean = false) {
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
for (loc <- tasks(index).preferredLocations) {
loc match {
case e: ExecutorCacheTaskLocation =>
Expand All @@ -234,8 +246,13 @@ private[spark] class TaskSetManager(
case _ =>
}
pendingTasksForHost.getOrElseUpdate(loc.host, new ArrayBuffer) += index
for (rack <- sched.getRackForHost(loc.host)) {
pendingTasksForRack.getOrElseUpdate(rack, new ArrayBuffer) += index

if (initialing) {
locationWithTaskIndex += ((loc.host, index))
} else {
for (rack <- sched.getRackForHost(loc.host)) {
pendingTasksForRack.getOrElseUpdate(rack, new ArrayBuffer) += index
}
}
}

Expand All @@ -249,24 +266,27 @@ private[spark] class TaskSetManager(
/**
* Return the pending tasks list for a given executor ID, or an empty list if
* there is no map entry for that host
* This is visible for testing.
*/
private def getPendingTasksForExecutor(executorId: String): ArrayBuffer[Int] = {
private[scheduler] def getPendingTasksForExecutor(executorId: String): ArrayBuffer[Int] = {
pendingTasksForExecutor.getOrElse(executorId, ArrayBuffer())
}

/**
* Return the pending tasks list for a given host, or an empty list if
* there is no map entry for that host
* This is visible for testing.
*/
private def getPendingTasksForHost(host: String): ArrayBuffer[Int] = {
private[scheduler] def getPendingTasksForHost(host: String): ArrayBuffer[Int] = {
pendingTasksForHost.getOrElse(host, ArrayBuffer())
}

/**
* Return the pending rack-local task list for a given rack, or an empty list if
* there is no map entry for that rack
* This is visible for testing.
*/
private def getPendingTasksForRack(rack: String): ArrayBuffer[Int] = {
private[scheduler] def getPendingTasksForRack(rack: String): ArrayBuffer[Int] = {
pendingTasksForRack.getOrElse(rack, ArrayBuffer())
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import java.util.{Properties, Random}
import scala.collection.mutable
import scala.collection.mutable.ArrayBuffer

import org.mockito.ArgumentMatchers.{any, anyInt, anyString}
import org.mockito.ArgumentMatchers.{any, anyBoolean, anyInt, anyString}
import org.mockito.Mockito.{mock, never, spy, times, verify, when}
import org.mockito.invocation.InvocationOnMock
import org.mockito.stubbing.Answer
Expand Down Expand Up @@ -78,9 +78,19 @@ object FakeRackUtil {
hostToRack(host) = rack
}

def getRackForHost(host: String): Option[String] = {
def getRackForHost(host: String, slow: Boolean = false): Option[String] = {
if (slow) {
Thread.sleep(100) // assume resolving one host takes 100 ms
}
hostToRack.get(host)
}

def getRacksForHosts(hosts: List[String], slow: Boolean = false): List[String] = {
if (slow) {
Thread.sleep(500) // assume resolving multiple hosts takes 500 ms
}
hosts.flatMap(hostToRack.get)
}
}

/**
Expand All @@ -97,6 +107,7 @@ class FakeTaskScheduler(sc: SparkContext, liveExecutors: (String, String)* /* ex
val finishedManagers = new ArrayBuffer[TaskSetManager]
val taskSetsFailed = new ArrayBuffer[String]
val speculativeTasks = new ArrayBuffer[Int]
var slowRackResolve = false

val executors = new mutable.HashMap[String, String]
for ((execId, host) <- liveExecutors) {
Expand Down Expand Up @@ -145,7 +156,11 @@ class FakeTaskScheduler(sc: SparkContext, liveExecutors: (String, String)* /* ex
}


override def getRackForHost(value: String): Option[String] = FakeRackUtil.getRackForHost(value)
override def getRackForHost(value: String): Option[String] =
FakeRackUtil.getRackForHost(value, slowRackResolve)

override def getRacksForHosts(values: List[String]): List[String] =
FakeRackUtil.getRacksForHosts(values, slowRackResolve)
}

/**
Expand Down Expand Up @@ -1316,7 +1331,7 @@ class TaskSetManagerSuite extends SparkFunSuite with LocalSparkContext with Logg
val taskDesc = taskSetManagerSpy.resourceOffer(exec, host, TaskLocality.ANY)

// Assert the task has been black listed on the executor it was last executed on.
when(taskSetManagerSpy.addPendingTask(anyInt())).thenAnswer(
when(taskSetManagerSpy.addPendingTask(anyInt(), anyBoolean())).thenAnswer(
new Answer[Unit] {
override def answer(invocationOnMock: InvocationOnMock): Unit = {
val task: Int = invocationOnMock.getArgument(0)
Expand All @@ -1330,7 +1345,7 @@ class TaskSetManagerSuite extends SparkFunSuite with LocalSparkContext with Logg
val e = new ExceptionFailure("a", "b", Array(), "c", None)
taskSetManagerSpy.handleFailedTask(taskDesc.get.taskId, TaskState.FAILED, e)

verify(taskSetManagerSpy, times(1)).addPendingTask(anyInt())
verify(taskSetManagerSpy, times(1)).addPendingTask(anyInt(), anyBoolean())
}

test("SPARK-21563 context's added jars shouldn't change mid-TaskSet") {
Expand Down Expand Up @@ -1602,4 +1617,28 @@ class TaskSetManagerSuite extends SparkFunSuite with LocalSparkContext with Logg
verify(sched.dagScheduler).taskEnded(manager.tasks(3), Success, result.value(),
result.accumUpdates, info3)
}

test("SPARK-27038: Verify the rack resolving time and result when initialing TaskSetManager") {
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
sc = new SparkContext("local", "test")
for (i <- 1 to 100) {
FakeRackUtil.assignHostToRack("host" + i, "rack" + i)
}
sched = new FakeTaskScheduler(sc,
("execA", "host1"), ("execB", "host2"), ("execC", "host3"))
sched.slowRackResolve = true
val locations = new ArrayBuffer[Seq[TaskLocation]]()
for (i <- 1 to 100) {
locations += Seq(TaskLocation("host" + i))
}
val taskSet = FakeTask.createTaskSet(100, locations: _*)
val clock = new ManualClock
val manager = new TaskSetManager(sched, taskSet, MAX_TASK_FAILURES, clock = clock)
var total = 0
for (i <- 1 to 100) {
total += manager.getPendingTasksForRack("rack" + i).length
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
}
assert(total === 100) // verify the total number always equals 100 with/without SPARK-27038
// verify elapsed time should be less than 1s, without SPARK-27038, it should be larger 10s
assert(manager.addTaskElapsedTime < 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is going to be super flaky on jenkins. I've seen tests randomly pause for multiple seconds.

Rather than testing the timing, I'd count how many times sched.getRackForHost etc. are called.

@LantaoJin LantaoJin Mar 7, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @squito , I changed it with a simulation function of org.apache.hadoop.net.ScriptBasedMapping.runResolveCommand(). I think it could equal to time reducing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to counting how many times getRacksForHosts

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* 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 scala.collection.JavaConverters._
import scala.collection.mutable.ArrayBuffer

import com.google.common.base.Strings
import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.fs.CommonConfigurationKeysPublic
import org.apache.hadoop.net._
import org.apache.hadoop.util.ReflectionUtils

import org.apache.spark.internal.Logging

/**
* Added in SPARK-27038. Before using the higher Hadoop version which applied YARN-9332,
* we construct [[YarnRackResolver]] instead of [[org.apache.hadoop.yarn.util.RackResolver]]
* to revolve the rack info.
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
*/
object YarnRackResolver extends Logging {
private var dnsToSwitchMapping: DNSToSwitchMapping = _
private var initCalled = false
// advisory count of arguments for rack script
private val ADVISORY_MINIMUM_NUMBER_SCRIPT_ARGS = 10000

def init(conf: Configuration): Unit = {
if (!initCalled) {
initCalled = true
val dnsToSwitchMappingClass =
conf.getClass(CommonConfigurationKeysPublic.NET_TOPOLOGY_NODE_SWITCH_MAPPING_IMPL_KEY,
classOf[ScriptBasedMapping], classOf[DNSToSwitchMapping])
if (classOf[ScriptBasedMapping].isAssignableFrom(dnsToSwitchMappingClass)) {
val numArgs = conf.getInt(CommonConfigurationKeysPublic.NET_TOPOLOGY_SCRIPT_NUMBER_ARGS_KEY,
CommonConfigurationKeysPublic.NET_TOPOLOGY_SCRIPT_NUMBER_ARGS_DEFAULT)
if (numArgs < ADVISORY_MINIMUM_NUMBER_SCRIPT_ARGS) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand the numArgs part at all, can you please explain?

@LantaoJin LantaoJin Mar 7, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the max allowed count of script arguments in one execution. In Hadoop, the length of script arguments over numArgs will be executed in next round in loop. The default value 100 means hosts over 100 couldn't be resolved in one shot. For instance, we have 201 hosts to be resolved for a job, Hadoop will launch script three times to resolve:

script.py host1 host2 ... host100
script.py host101... host200, host201
script.py host201

It launches this python script 3 times. First is for host1 to host100, the second round resolves host100 to host200, the last round only resolves host201.

For Spark, in a big cluster, we hope to reduce the script execution rounds. 100 may be too small. In a small cluster or small job, whatever this value is 100 or 10000, there is no harm. But in for a large cluster with a big job. Logging out this warning log is to remind users to increase the configuration net.topology.script.number.args to reduce the inefficient execution loops of script.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the value 10000 I set hasn't any evidence from production testing. I just think it could be resolved in one execution shot and I think almost clusters in industry are under 10000 nodes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the warning log doesn't give user any gold value for it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this has no effect on how you prepare the arguments to this script in spark itself, it just controls the behavior of that script, right?

so is there a downside to running with a huge number, say 10M? Not knowing anything about that script, I figure there must be some tradeoff here. I am not sure you should be setting the value to 10K if you haven't tried anything which hits that limit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't any existing "the script" in Hadoop or Spark. This needs to be provided by Hadoop administrator. If it is not set, the default value of DEFAULT_RACK is returned for all node names. But whatever the script looks like, the interface runResolveCommand in ScriptBasedMapping is fixed: It accepts a host list and executes several times based on net.topology.script.number.args we mentioned above. So I didn't take account of any execution performance of this script. Logging with a higher advisory value is based on the character of Spark jobs comparing to MR job. I will change this logging to INFO and remove the fixed value 10k.

logWarning(s"Increasing the value of" +
s" ${CommonConfigurationKeysPublic.NET_TOPOLOGY_SCRIPT_NUMBER_ARGS_KEY} could reduce" +
s" the time of rack resolving when submits a stage with a mass of tasks." +
s" Current number is $numArgs")
}
}
try {
val newInstance = ReflectionUtils.newInstance(dnsToSwitchMappingClass, conf)
.asInstanceOf[DNSToSwitchMapping]
dnsToSwitchMapping = newInstance match {
case _: CachedDNSToSwitchMapping => newInstance
case _ => new CachedDNSToSwitchMapping(newInstance)
}
} catch {
case e: Exception =>
throw new RuntimeException(e)
}
}
}

def resolveRacks(conf: Configuration, hostNames: List[String]): List[Node] = {
init(conf)
val nodes = new ArrayBuffer[Node]
val rNameList = dnsToSwitchMapping.resolve(hostNames.toList.asJava).asScala
if (rNameList == null || rNameList.isEmpty) {
hostNames.foreach(nodes += new NodeBase(_, NetworkTopology.DEFAULT_RACK))
logInfo(s"Got an error when resolve hostNames. " +
s"Falling back to ${NetworkTopology.DEFAULT_RACK} for all")
} else {
for ((hostName, rName) <- hostNames.zip(rNameList)) {
if (Strings.isNullOrEmpty(rName)) {
// fallback to use default rack
nodes += new NodeBase(hostName, NetworkTopology.DEFAULT_RACK)
logDebug(s"Could not resolve $hostName. " +
s"Falling back to ${NetworkTopology.DEFAULT_RACK}")
} else {
nodes += new NodeBase(hostName, rName)
}
}
}
nodes.toList
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@

package org.apache.spark.scheduler.cluster

import org.apache.hadoop.net.{NetworkTopology, NodeBase}
import org.apache.hadoop.yarn.util.RackResolver
import org.apache.log4j.{Level, Logger}

import org.apache.spark._
import org.apache.spark.internal.config.LOCALITY_WAIT
import org.apache.spark.scheduler.TaskSchedulerImpl
import org.apache.spark.util.Utils

Expand All @@ -31,9 +33,33 @@ private[spark] class YarnScheduler(sc: SparkContext) extends TaskSchedulerImpl(s
Logger.getLogger(classOf[RackResolver]).setLevel(Level.WARN)
}

// Add a on-off switch to save time for rack resolving
private val skipRackResolving = sc.conf.getTimeAsMs(
"spark.locality.wait.rack", sc.conf.get(LOCALITY_WAIT).toString) == 0

// By default, rack is unknown
override def getRackForHost(hostPort: String): Option[String] = {
val host = Utils.parseHostPort(hostPort)._1
Option(RackResolver.resolve(sc.hadoopConfiguration, host).getNetworkLocation)
if (skipRackResolving) {
Option(NetworkTopology.DEFAULT_RACK)
} else {
val host = Utils.parseHostPort(hostPort)._1
Option(RackResolver.resolve(sc.hadoopConfiguration, host).getNetworkLocation)
}
}

/**
* Get racks info for a list of host.
* Use [[YarnRackResolver]] to resolve racks instead of [[RackResolver]]
* before the dependency Hadoop applied YARN-9332
* @param hostPorts host list to resolve
* @return rack list
*/
override def getRacksForHosts(hostPorts: List[String]): List[String] = {
val hosts = hostPorts.map(Utils.parseHostPort(_)._1)
if (skipRackResolving) {
hosts.map(new NodeBase(_, NetworkTopology.DEFAULT_RACK)).map(_.getNetworkLocation)
} else {
YarnRackResolver.resolveRacks(sc.hadoopConfiguration, hosts).map(_.getNetworkLocation)
}
}
}