-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-13704][CORE][YARN] Re-implement RackResolver to reduce resolving time #23951
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
Changes from 4 commits
56b7f70
342a6fd
1e1180c
e521bcf
8514b3e
63666ad
69b62e4
6246e57
7e4c729
d4a7cde
d3e1592
47add6f
e8ab99e
11fdd41
ade4caa
c9dace8
e2faee6
aca97f1
75d22ed
011b4c0
2876ad3
06a6264
92ef335
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)]() | ||
|
LantaoJin marked this conversation as resolved.
Outdated
|
||
| private val addTaskStartTime = System.nanoTime() | ||
|
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) | ||
|
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. 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
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. btw I meant something like this for avoiding leaving around 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
Contributor
Author
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.
We can not de-duping this List, or the logic about choosing a task to launch will incorrect.
Contributor
Author
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.
Yes, I don't want to do any de-duping here. I will refactor this part for getting more readable.
Contributor
Author
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. Refactored
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. 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 so after looking at that code, I'm more convinced de-duping would help you.
Contributor
Author
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. Refactor with the de-duping approach. |
||
| .zip(locationWithTaskIndex.map(_._2)) | ||
| rackWithTaskIndex.foreach { | ||
| case (Some(rack), index) => | ||
| pendingTasksForRack.getOrElseUpdate(rack, new ArrayBuffer) += index | ||
| case _ => | ||
| } | ||
| logInfo(s"Adding pending tasks take ${(System.nanoTime() - addTaskStartTime) / 1e9} seconds") | ||
|
|
||
| /** | ||
| * Track the set of locality levels which are valid given the tasks locality preferences and | ||
|
|
@@ -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, initializing: Boolean = false) { | ||
| for (loc <- tasks(index).preferredLocations) { | ||
| loc match { | ||
| case e: ExecutorCacheTaskLocation => | ||
|
|
@@ -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 (initializing) { | ||
| locationWithTaskIndex += ((loc.host, index)) | ||
| } else { | ||
| for (rack <- sched.getRackForHost(loc.host)) { | ||
| pendingTasksForRack.getOrElseUpdate(rack, new ArrayBuffer) += index | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -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()) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -69,18 +69,48 @@ class FakeDAGScheduler(sc: SparkContext, taskScheduler: FakeTaskScheduler) | |
| // Get the rack for a given host | ||
| object FakeRackUtil { | ||
| private val hostToRack = new mutable.HashMap[String, String]() | ||
| var loopCount = 0 | ||
|
|
||
| def cleanUp() { | ||
| hostToRack.clear() | ||
| loopCount = 0 | ||
| } | ||
|
|
||
| def assignHostToRack(host: String, rack: String) { | ||
| hostToRack(host) = rack | ||
| } | ||
|
|
||
| def getRackForHost(host: String): Option[String] = { | ||
| loopCount = simulateRunResolveCommand(Seq(host)) | ||
| hostToRack.get(host) | ||
| } | ||
|
|
||
| def getRacksForHosts(hosts: List[String]): List[Option[String]] = { | ||
| loopCount = simulateRunResolveCommand(hosts) | ||
| hosts.map(hostToRack.get) | ||
| } | ||
|
|
||
| /** | ||
| * This is a simulation of building and executing the resolution command. | ||
| * Simulate function `runResolveCommand()` in [[org.apache.hadoop.net.ScriptBasedMapping]]. | ||
| * If Seq has 100 elements, it returns 4. If Seq has 1 elements, it returns 1. | ||
| * @param args a list of arguments | ||
| * @return script execution times | ||
| */ | ||
| private def simulateRunResolveCommand(args: Seq[String]): Int = { | ||
| val maxArgs = 30 // Simulate NET_TOPOLOGY_SCRIPT_NUMBER_ARGS_DEFAULT | ||
| var numProcessed = 0 | ||
| var loopCount = 0 | ||
| while (numProcessed != args.size) { | ||
| var start = maxArgs * loopCount | ||
| numProcessed = start | ||
| while (numProcessed < (start + maxArgs) && numProcessed < args.size) { | ||
| numProcessed += 1 | ||
| } | ||
| loopCount += 1 | ||
| } | ||
| loopCount | ||
|
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. is there really any value in adding this complexity to the test? Stepping back a bit, I think there are 2 changes you're making here to improve performance:
I think here you just want to test (1), and for that, you just need to count invocations for Would it be worth also adding a test for (2) somehow? I'm not sure what you could do there without it being tied to the internals of the hadoop logic. You could test the instance of the created Also I think it would be easy and useful for you to add a test for (3) here, the same way you're testing (1), just that you'd expect
Contributor
Author
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. About (1) Why I added this complexity code for UT is that after applied this patch, the initializing processing of
Contributor
Author
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. About (2) This part has tested in Hadoop and I just create an instance of So I think it doesn't need to add a test for the cache.
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.
Just count the invocations in object FakeRackUtil {
...
def getRackForHost(host: String): Option[String] = {
getRackForHostCount += 1
hostToRack.get(host)
}
...
test("SPARK-27038 Verify the rack resolving time has been reduced") {
FakeRackUtil.getRackForHostCount = 0
FakeRackUtil.getRackForHostsCount = 0
...
assert(FakeRackUtil.getRackForHostCount === 0)
assert(FakeRackUtil.getRackForHostsCount === 1)
}
Contributor
Author
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.
|
||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -97,6 +127,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) { | ||
|
|
@@ -145,7 +176,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) | ||
|
|
||
| override def getRacksForHosts(values: List[String]): List[Option[String]] = | ||
| FakeRackUtil.getRacksForHosts(values) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1316,7 +1351,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) | ||
|
|
@@ -1330,7 +1365,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") { | ||
|
|
@@ -1602,4 +1637,27 @@ 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 has been reduced") { | ||
| 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 | ||
|
LantaoJin marked this conversation as resolved.
Outdated
|
||
| } | ||
| assert(total === 100) // verify the total number not changed with SPARK-27038 | ||
|
LantaoJin marked this conversation as resolved.
Outdated
|
||
| assert(FakeRackUtil.loopCount == 4) // verify script execution loop count decreased | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,18 @@ | |
|
|
||
| package org.apache.spark.deploy.yarn | ||
|
|
||
| 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.hadoop.yarn.util.RackResolver | ||
| import org.apache.log4j.{Level, Logger} | ||
|
|
||
| import org.apache.spark.internal.Logging | ||
|
|
||
| /** | ||
| * Wrapper around YARN's [[RackResolver]]. This allows Spark tests to easily override the | ||
| * default behavior, since YARN's class self-initializes the first time it's called, and | ||
|
|
@@ -38,3 +46,72 @@ private[yarn] class SparkRackResolver { | |
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Utility to resolve the rack for hosts in an efficient manner. | ||
| * It will cache the rack for individual hosts to avoid | ||
| * repeatedly performing the same expensive lookup. | ||
| * | ||
| * This will be unnecessary in hadoop releases with YARN-9332. | ||
| * With that, we could just directly use [[org.apache.hadoop.yarn.util.RackResolver]]. | ||
| * In the meantime, this is a re-implementation for spark's use. | ||
| */ | ||
| object SparkRackResolver extends Logging { | ||
|
LantaoJin marked this conversation as resolved.
Outdated
|
||
| private var dnsToSwitchMapping: DNSToSwitchMapping = _ | ||
| private var initCalled = false | ||
| // advisory count of arguments for rack script, less than this will get a warning | ||
| private val ADVISORY_MINIMUM_NUMBER_SCRIPT_ARGS = 10000 | ||
|
|
||
| def init(conf: Configuration): Unit = { | ||
| if (!initCalled) { | ||
|
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. This doesn't look thread-safe. You can have this called from at least two different threads in cluster mode (the task scheduler and the YarnAllocator). I just want to say again that I dislike this object, because caching things in static fields is generally a bad idea. In this case it allows you to share that state between the two callers I mention above, but it also means that if you start multiple Spark apps in the same JVM, the first one will be the one that defined what this configuration will be. I'll let this object issue slide for now since that is less common of a use case (and the underlying YARN code already had this problem before this change anyway), but the thread-safety needs to be fixed.
Contributor
Author
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 I use static fields here is for the cache
Contributor
Author
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've added the sync block to fix thread-safe problem.
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. As Bruce mentioned this is not thread-safe. You want: You don't need the boolean since you always initialize
I know. What I'm saying is that it's generally a bad idea. e.g. you can't test this with different configurations, for example, because once the first test runs, this is initialized forever. I'm not saying caching is bad. I'm saying using static variables for caching is bad. But anyway, this is already a problem because YARN does that sort of thing, so this is no worse than before. But since we're here, you could:
That's less icky because it at least allows you to create a new resolver with a different config if you really want to.
Contributor
Author
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. Every calling of
Contributor
Author
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. Meanwhile, since
Contributor
Author
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. Let me fix the thread-safe problem first.
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. The It allows the caching you want while keeping the logic where it belongs, in the class. Everybody who uses the object will get the cached version; the cached instance is initialized in the first call. Just like your code here, but much simpler. And it allows those who want to instantiate a separate resolver with a separate config if they so choose.
Contributor
Author
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 I understand now. I will update it
Contributor
Author
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. Refactored. |
||
| 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) { | ||
| 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) | ||
|
LantaoJin marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| 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 | ||
|
LantaoJin marked this conversation as resolved.
Outdated
|
||
| 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 | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.