[SPARK-17894] [CORE] Ensure uniqueness of TaskSetManager name. #15463

erenavsarogullari · 2016-10-13T11:49:58Z

What changes were proposed in this pull request?

TaskSetManager should have unique name to avoid adding duplicate ones to parent Pool via SchedulableBuilder. This problem has been surfaced with following discussion: [PR: Avoid adding duplicate schedulables]

Proposal :
There is 1x1 relationship between stageAttemptId and TaskSetManager so taskSet.Id covering both stageId and stageAttemptId looks to be used for uniqueness of TaskSetManager name instead of just stageId.

Current TaskSetManager Name :
var name = "TaskSet_" + taskSet.stageId.toString
Sample: TaskSet_0

Proposed TaskSetManager Name :
val name = "TaskSet_" + taskSet.Id // taskSet.Id = (stageId + "." + stageAttemptId)
Sample : TaskSet_0.0

How was this patch tested?

Added new Unit Test.

erenavsarogullari · 2016-10-19T18:30:03Z

cc @kayousterhout @markhamstra

kayousterhout

Thanks for fixing this!

Jenkins, this is OK to test.

kayousterhout · 2016-10-19T18:42:53Z

core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala

Can you make this a little more specific and also include the JIRA: "[SPARK-17894] Verify TaskSetManagers for different stage attempts have unique names"

kayousterhout · 2016-10-19T18:43:42Z

core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala

Can you pass these in as named parameters, to make the tests a little easier to read?

taskSet = FakeTask.createTaskSet(numTasks = 1, stageId = 0, stageAttemptId = 0)

kayousterhout · 2016-10-19T18:44:26Z

core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala

Can you add a comment about what's going on in this chunk -- something like "Make sure a task set for the same stage (but a different attempt) has a different ID"

kayousterhout · 2016-10-19T18:44:57Z

core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala

Similar here -- "// Make sure a task set with the same attempt ID but different stage ID also has a unique name"

kayousterhout · 2016-10-19T18:47:54Z

core/src/test/scala/org/apache/spark/scheduler/FakeTask.scala

can you pass 0 as a named parameter ("stageId = 0") to make it obvious what it's being used for?

erenavsarogullari · 2016-10-19T21:38:36Z

Thanks @kayousterhout for the comments. They are addressed.

kayousterhout · 2016-10-19T22:18:43Z

Jenkins, test this please

erenavsarogullari · 2016-10-20T21:12:49Z

Thanks @kayousterhout for review again. Jenkins did not start to build and run the tests.

kayousterhout · 2016-10-20T21:14:18Z

Jenkins, this is OK to test

erenavsarogullari · 2016-10-21T15:17:40Z

I think Jenkins can not be triggered ;) Is there any other way to trigger?

kayousterhout · 2016-10-21T15:23:49Z

Jenkins test this please

erenavsarogullari · 2016-10-21T16:26:52Z

Thanks @kayousterhout for support but I think problem still continues. Is there any contact from infrastructure/jenkins team to get support?

shivaram · 2016-10-21T20:43:14Z

Jenkins, retest this please

markhamstra · 2016-10-21T20:50:37Z

Whoa! Jenkins disrespects @kayousterhout -- bad Jenkins! Or did you actually fix something @shivaram ?

shivaram · 2016-10-21T20:53:16Z

Hah - I wish I had that much insight into Jenkins. I just think it was just flaky esp. given the github DNS issues.

SparkQA · 2016-10-22T00:57:34Z

Test build #67349 has finished for PR 15463 at commit cd6d240.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

erenavsarogullari · 2016-10-22T10:17:24Z

Thanks all for the support. Jenkins looks failed due to timeout.
Jenkins, retest this please

shivaram · 2016-10-22T23:31:45Z

Jenkins, retest this please

SparkQA · 2016-10-23T01:50:58Z

Test build #67399 has finished for PR 15463 at commit cd6d240.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

erenavsarogullari · 2016-10-24T18:04:22Z

Thanks all for the support.
Patch looks passed on Jenkins so it is ready to merge if there is no extra comments ;)

kayousterhout · 2016-10-24T22:35:03Z

LGTM; merged to master

zsxwing · 2016-10-25T00:44:26Z

core/src/test/scala/org/apache/spark/scheduler/FakeTask.scala

    stageId: Int,
    partitionId: Int,
-    prefLocs: Seq[TaskLocation] = Nil) extends Task[Int](stageId, 0, partitionId) {
+    prefLocs: Seq[TaskLocation] = Nil) extends Task[Int](stageId, stageAttemptId = 0, partitionId) {


This is not supported in Scala 2.10. Could you submit a follow-up PR to fix the Scala 2.10 build, please? Thanks!

See: https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-master-compile-sbt-scala-2.10/2914/console

Just opened #15617 sorry about this!

`TaskSetManager` should have unique name to avoid adding duplicate ones to parent `Pool` via `SchedulableBuilder`. This problem has been surfaced with following discussion: [[PR: Avoid adding duplicate schedulables]](apache#15326) **Proposal** : There is 1x1 relationship between `stageAttemptId` and `TaskSetManager` so `taskSet.Id` covering both `stageId` and `stageAttemptId` looks to be used for uniqueness of `TaskSetManager` name instead of just `stageId`. **Current TaskSetManager Name** : `var name = "TaskSet_" + taskSet.stageId.toString` **Sample**: TaskSet_0 **Proposed TaskSetManager Name** : `val name = "TaskSet_" + taskSet.Id ` `// taskSet.Id = (stageId + "." + stageAttemptId)` **Sample** : TaskSet_0.0 Added new Unit Test. Author: erenavsarogullari <[email protected]> Closes apache#15463 from erenavsarogullari/SPARK-17894.

kayousterhout reviewed Oct 19, 2016

View reviewed changes

erenavsarogullari added 2 commits October 19, 2016 20:59

Ensure uniqueness of TaskSetManager name.

08526a8

Comments are addressed.

cd6d240

asfgit closed this in 81d6933 Oct 24, 2016

zsxwing reviewed Oct 25, 2016

View reviewed changes

erenavsarogullari mentioned this pull request Oct 25, 2016

[SPARK-17759] [CORE] Avoid adding duplicate schedulables #15326

Closed

erenavsarogullari deleted the SPARK-17894 branch October 26, 2016 20:37

[SPARK-17894] [CORE] Ensure uniqueness of TaskSetManager name. #15463

[SPARK-17894] [CORE] Ensure uniqueness of TaskSetManager name. #15463

Uh oh!

Conversation

erenavsarogullari commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

erenavsarogullari commented Oct 19, 2016

Uh oh!

kayousterhout left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erenavsarogullari commented Oct 19, 2016

Uh oh!

kayousterhout commented Oct 19, 2016

Uh oh!

erenavsarogullari commented Oct 20, 2016

Uh oh!

kayousterhout commented Oct 20, 2016

Uh oh!

erenavsarogullari commented Oct 21, 2016

Uh oh!

kayousterhout commented Oct 21, 2016

Uh oh!

erenavsarogullari commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shivaram commented Oct 21, 2016

Uh oh!

markhamstra commented Oct 21, 2016

Uh oh!

shivaram commented Oct 21, 2016

Uh oh!

SparkQA commented Oct 22, 2016

Uh oh!

erenavsarogullari commented Oct 22, 2016

Uh oh!

shivaram commented Oct 22, 2016

Uh oh!

SparkQA commented Oct 23, 2016

Uh oh!

erenavsarogullari commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayousterhout commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erenavsarogullari commented Oct 13, 2016 •

edited

Loading

erenavsarogullari commented Oct 21, 2016 •

edited

Loading

erenavsarogullari commented Oct 24, 2016 •

edited

Loading