Part of [SPARK-2456] Removed some HashMaps from DAGScheduler by storing information in Stage. #1561
Conversation
|
QA tests have started for PR 1561. This patch merges cleanly. |
There was a problem hiding this comment.
nit: It's a Set not a List
There was a problem hiding this comment.
as long as we're nit-ing..."belong" -> "belongs"
|
QA results for PR 1561: |
There was a problem hiding this comment.
Why filter instead of filterKeys? Looks to me like that only serves to make the predicate harder to read.
There was a problem hiding this comment.
i'd need to do more hash lookups if it is filterKeys
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I need the stage / value object in the foreach after the filter.
There was a problem hiding this comment.
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 { ... }.
There was a problem hiding this comment.
Actually that's pretty cool. I looked into the source code more. Didn't realize the whole FilteredKeys thing is "lazy".
|
JIRA? |
|
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)? |
There was a problem hiding this comment.
Why not just do this as part of the Stage constructor?
There was a problem hiding this comment.
And as a result of the above, you can consolidate this if/else into one thing
|
Love the cleanup here!!! |
There was a problem hiding this comment.
do i need to call stage.pendingTasks = new HashSet here? @kayousterhout @markhamstra
There was a problem hiding this comment.
Actually you should clear it here, I think the issue is if a stage gets resubmitted.
|
Thanks. Pushed a version that should have addressed most of the comments. |
|
QA tests have started for PR 1561. This patch merges cleanly. |
|
QA results for PR 1561: |
|
QA tests have started for PR 1561. This patch merges cleanly. |
|
QA results for PR 1561: |
|
Looks good to me, this is definitely cleaner |
|
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. |
|
That makes sense. I was wondering about it myself too. |
|
QA tests have started for PR 1561. This patch merges cleanly. |
|
QA results for PR 1561: |
|
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 |
|
@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? |
|
@markhamstra Good point; an abstract class is fine, too. |
|
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). |
|
QA tests have started for PR 1561. This patch merges cleanly. |
|
QA results for PR 1561: |
|
Jenkins, retest this please. |
|
hive-thriftserver test failed. |
|
I'd be okay deferring the subclass thing till later too, the benefit isn't huge right now. |
|
QA tests have started for PR 1561. This patch merges cleanly. |
|
QA results for PR 1561: |
|
Merging this in master. Thanks for reviewing. |
…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.
This is part of the scheduler cleanup/refactoring effort to make the scheduler code easier to maintain.
@kayousterhout @markhamstra please take a look ...