Part of [SPARK-2456] Removed some HashMaps from DAGScheduler by storing information in Stage. by rxin · Pull Request #1561 · apache/spark

rxin · 2014-07-24T02:41:52Z

This is part of the scheduler cleanup/refactoring effort to make the scheduler code easier to maintain.

@kayousterhout @markhamstra please take a look ...

SparkQA · 2014-07-24T02:43:22Z

QA tests have started for PR 1561. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17087/consoleFull

markhamstra · 2014-07-24T03:01:02Z

nit: It's a Set not a List

as long as we're nit-ing..."belong" -> "belongs"

SparkQA · 2014-07-24T03:26:07Z

QA results for PR 1561:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17087/consoleFull

markhamstra · 2014-07-24T03:27:37Z

Why filter instead of filterKeys? Looks to me like that only serves to make the predicate harder to read.

i'd need to do more hash lookups if it is filterKeys

What? How does stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach { do more hash lookups than does stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {? Looks the same to me: https://github.com/scala/scala/blob/v2.10.4/src/library/scala/collection/MapLike.scala#L230

I need the stage / value object in the foreach after the filter.

Yes, and you've got it after filterKeys just like after filter. The result of filterKeys is a Map[Int, Stage], and nothing needs to change within the foreach { ... }.

Actually that's pretty cool. I looked into the source code more. Didn't realize the whole FilteredKeys thing is "lazy".

markhamstra · 2014-07-24T04:48:11Z

JIRA?

kayousterhout · 2014-07-24T05:52:41Z

Total nit but would you mind adding to the description (so it ends up in the commit) the point of this change (which, I think, is as part of a scheduler cleanup effort)?

kayousterhout · 2014-07-24T05:53:19Z

Why not just do this as part of the Stage constructor?

kayousterhout · 2014-07-24T06:11:37Z

And as a result of the above, you can consolidate this if/else into one thing

kayousterhout · 2014-07-24T06:23:39Z

Love the cleanup here!!!

rxin · 2014-07-24T08:09:41Z

do i need to call stage.pendingTasks = new HashSet here? @kayousterhout @markhamstra

Actually you should clear it here, I think the issue is if a stage gets resubmitted.

rxin · 2014-07-24T08:10:01Z

Thanks. Pushed a version that should have addressed most of the comments.

SparkQA · 2014-07-24T08:13:25Z

QA tests have started for PR 1561. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17112/consoleFull

SparkQA · 2014-07-24T08:55:56Z

QA results for PR 1561:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17112/consoleFull

SparkQA · 2014-07-25T02:08:31Z

QA tests have started for PR 1561. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17157/consoleFull

SparkQA · 2014-07-25T02:51:24Z

QA results for PR 1561:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17157/consoleFull

mateiz · 2014-07-25T17:56:33Z

Looks good to me, this is definitely cleaner

mateiz · 2014-07-25T18:07:35Z

Actually I made one note on clearing pendingTasks. Probably best to keep that code. I think it might come in if a stage is resubmitted.

rxin · 2014-07-25T18:10:41Z

That makes sense. I was wondering about it myself too.

SparkQA · 2014-07-25T18:13:45Z

QA tests have started for PR 1561. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17191/consoleFull

SparkQA · 2014-07-25T19:04:03Z

QA results for PR 1561:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17191/consoleFull

JoshRosen · 2014-07-25T20:27:19Z

A lot of the methods and data structures in Stage are specific to only ShuffleMapStage or ResultStage. For example, we only track output locations for ShuffleMapStages and only have ActiveJobs for ResultStages. What do you think about splitting Stage into a trait and two subclasses? This could improve the understandability of other DAGScheduler data structures; for example, we have

shuffleToMapStage = new HashMap[Int, Stage]

which only holds ShuffleMapStages, so we could give it a more specific type of HashMap[Int, ShuffleMapStage].

markhamstra · 2014-07-25T20:35:59Z

@JoshRosen Why a trait instead of an abstract class? We're not expecting to need to mixin Stage outside of the Stage class hierarchy, right?

JoshRosen · 2014-07-25T20:39:37Z

@markhamstra Good point; an abstract class is fine, too.

rxin · 2014-07-25T21:00:54Z

That's a good discussion to have. I'm personally in favor of keeping it simple for now until it becomes more complicated. Refactoring common parts are good, but the extra level of indirection also introduces complexity. As I see it right now, the benefit isn't huge yet (it's only a couple variables that we are tracking).

SparkQA · 2014-07-25T21:08:38Z

QA tests have started for PR 1561. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17199/consoleFull

SparkQA · 2014-07-25T21:55:41Z

QA results for PR 1561:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17199/consoleFull

rxin · 2014-07-25T21:59:05Z

Jenkins, retest this please.

rxin · 2014-07-25T21:59:10Z

hive-thriftserver test failed.

mateiz · 2014-07-25T22:10:08Z

I'd be okay deferring the subclass thing till later too, the benefit isn't huge right now.

SparkQA · 2014-07-26T00:58:31Z

QA tests have started for PR 1561. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17210/consoleFull

SparkQA · 2014-07-26T01:43:14Z

QA results for PR 1561:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17210/consoleFull

rxin · 2014-07-26T01:44:49Z

Merging this in master. Thanks for reviewing.

@kayousterhout

…ng information in Stage. This is part of the scheduler cleanup/refactoring effort to make the scheduler code easier to maintain. @kayousterhout @markhamstra please take a look ... Author: Reynold Xin <rxin@apache.org> Closes apache#1561 from rxin/dagSchedulerHashMaps and squashes the following commits: 1c44e15 [Reynold Xin] Clear pending tasks in submitMissingTasks. 620a0d1 [Reynold Xin] Use filterKeys. 5b54404 [Reynold Xin] Code review feedback. c1e9a1c [Reynold Xin] Removed some HashMaps from DAGScheduler by storing information in Stage.

markhamstra reviewed Jul 24, 2014
View reviewed changes

kayousterhout reviewed Jul 24, 2014
View reviewed changes

rxin changed the title ~~Removed some HashMaps from DAGScheduler by storing information in Stage.~~ [SPARK-2456] Removed some HashMaps from DAGScheduler by storing information in Stage. Jul 24, 2014

rxin changed the title ~~[SPARK-2456] Removed some HashMaps from DAGScheduler by storing information in Stage.~~ Part of [SPARK-2456] Removed some HashMaps from DAGScheduler by storing information in Stage. Jul 24, 2014

kayousterhout reviewed Jul 24, 2014
View reviewed changes

rxin reviewed Jul 24, 2014
View reviewed changes

markhamstra mentioned this pull request Jul 24, 2014

SPARK-1715: Ensure actor is self-contained in DAGScheduler #637

Closed

rxin added 4 commits July 25, 2014 17:56

Removed some HashMaps from DAGScheduler by storing information in Stage.

c1e9a1c

Code review feedback.

5b54404

Use filterKeys.

620a0d1

Clear pending tasks in submitMissingTasks.

1c44e15

asfgit closed this in 9d8666c Jul 26, 2014

rxin deleted the dagSchedulerHashMaps branch August 13, 2014 08:01

Uh oh!

Conversation

rxin commented Jul 24, 2014

Uh oh!

SparkQA commented Jul 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 24, 2014

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!

markhamstra commented Jul 24, 2014

Uh oh!

kayousterhout commented Jul 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayousterhout commented Jul 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Jul 24, 2014

Uh oh!

SparkQA commented Jul 24, 2014

Uh oh!

SparkQA commented Jul 24, 2014

Uh oh!

SparkQA commented Jul 25, 2014

Uh oh!

SparkQA commented Jul 25, 2014

Uh oh!

mateiz commented Jul 25, 2014

Uh oh!

mateiz commented Jul 25, 2014

Uh oh!

rxin commented Jul 25, 2014

Uh oh!

SparkQA commented Jul 25, 2014

Uh oh!

SparkQA commented Jul 25, 2014

Uh oh!

JoshRosen commented Jul 25, 2014

Uh oh!

markhamstra commented Jul 25, 2014

Uh oh!

JoshRosen commented Jul 25, 2014

Uh oh!

rxin commented Jul 25, 2014

Uh oh!

SparkQA commented Jul 25, 2014

Uh oh!

SparkQA commented Jul 25, 2014

Uh oh!

rxin commented Jul 25, 2014

Uh oh!

rxin commented Jul 25, 2014

Uh oh!

mateiz commented Jul 25, 2014

Uh oh!

SparkQA commented Jul 26, 2014

Uh oh!

SparkQA commented Jul 26, 2014

Uh oh!