You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rxin <gi...@git.apache.org> on 2014/07/24 04:41:55 UTC
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
GitHub user rxin opened a pull request:
https://github.com/apache/spark/pull/1561
Removed some HashMaps from DAGScheduler by storing information in Stage.
@kayousterhout @markhamstra please take a look ...
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rxin/spark dagSchedulerHashMaps
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1561.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1561
----
commit 76f85717acc49bb00d20bb4d3b59d74d2e934680
Author: Reynold Xin <rx...@apache.org>
Date: 2014-07-24T02:41:05Z
Removed some HashMaps from DAGScheduler by storing information in Stage.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49961566
QA tests have started for PR 1561. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17087/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50182962
Looks good to me, this is definitely cleaner
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50219833
QA results for PR 1561:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17210/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49978964
Thanks. Pushed a version that should have addressed most of the comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331819
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -22,6 +22,8 @@ import org.apache.spark.rdd.RDD
import org.apache.spark.storage.BlockManagerId
import org.apache.spark.util.CallSite
+import scala.collection.mutable.HashSet
--- End diff --
I'll have to remember that one!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328333
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -315,13 +309,14 @@ class DAGScheduler(
*/
private def updateJobIdStageIdMaps(jobId: Int, stage: Stage) {
def updateJobIdStageIdMapsList(stages: List[Stage]) {
- if (!stages.isEmpty) {
+ if (stages.nonEmpty) {
val s = stages.head
- stageIdToJobIds.getOrElseUpdate(s.id, new HashSet[Int]()) += jobId
+ s.jobIds += jobId
jobIdToStageIds.getOrElseUpdate(jobId, new HashSet[Int]()) += s.id
- val parents = getParentStages(s.rdd, jobId)
- val parentsWithoutThisJobId = parents.filter(p =>
- !stageIdToJobIds.get(p.id).exists(_.contains(jobId)))
+ val parents: List[Stage] = getParentStages(s.rdd, jobId)
--- End diff --
It is not obvious to me what getParentStages return just staring at the code. Sometimes when we refer to stages we use a set of integers, sometimes just Stage objects.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328358
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -395,16 +389,9 @@ class DAGScheduler(
activeJobs -= job
if (resultStage.isEmpty) {
--- End diff --
btw i'm confused by this actually. This function accepts an ActiveJob, which already has a link to the final stage. Why do we also accept resultStage? @kayousterhout @markhamstra
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331775
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -355,14 +351,13 @@ class DAGScheduler(
logDebug("Removing running stage %d".format(stageId))
runningStages -= stage
}
- stageToInfos -= stage
for ((k, v) <- shuffleToMapStage.find(_._2 == stage)) {
shuffleToMapStage.remove(k)
}
- if (pendingTasks.contains(stage) && !pendingTasks(stage).isEmpty) {
+ if (stage.pendingTasks.isDefined && !stage.pendingTasks.isEmpty) {
logDebug("Removing pending status for stage %d".format(stageId))
}
- pendingTasks -= stage
+ stage.pendingTasks = None
--- End diff --
The old pendingTasks needed the clearing step so that it wouldn't continue to have removed stages mapping to empty sets of remaining tasks. I agree that a similar step shouldn't be necessary now that pendingTasks is encapsulated in a Stage that is going away. In fact, in the event of resubmission don't we want this to _not_ be None, else stage.pendingTasks.get will fail?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15334329
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
logError("No stages registered for job " + job.jobId)
} else {
- stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach {
- case (stageId, jobSet) =>
+ stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {
--- End diff --
i'd need to do more hash lookups if it is filterKeys
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50100968
QA tests have started for PR 1561. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17157/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15361883
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
logError("No stages registered for job " + job.jobId)
} else {
- stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach {
- case (stageId, jobSet) =>
+ stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {
--- End diff --
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 { ... }.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50184871
QA tests have started for PR 1561. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17191/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15334478
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -728,8 +697,6 @@ class DAGScheduler(
private def submitMissingTasks(stage: Stage, jobId: Int) {
logDebug("submitMissingTasks(" + stage + ")")
// Get our pending tasks and remember them in our pendingTasks entry
- val myPending = pendingTasks.getOrElseUpdate(stage, new HashSet)
--- End diff --
do i need to call ```stage.pendingTasks = new HashSet``` here? @kayousterhout @markhamstra
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50201015
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50201403
@markhamstra Good point; an abstract class is fine, too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328544
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
--- End diff --
update the description of `@param resultStage` above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50210497
I'd be okay deferring the subclass thing till later too, the benefit isn't huge right now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1561
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328773
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -992,13 +974,14 @@ class DAGScheduler(
}
private[scheduler] def handleStageCancellation(stageId: Int) {
- if (stageIdToJobIds.contains(stageId)) {
- val jobsThatUseStage: Array[Int] = stageIdToJobIds(stageId).toArray
- jobsThatUseStage.foreach(jobId => {
- handleJobCancellation(jobId, "because Stage %s was cancelled".format(stageId))
- })
- } else {
- logInfo("No active jobs to kill for Stage " + stageId)
+ stageIdToStage.get(stageId) match {
+ case Some(stage) =>
+ val jobsThatUseStage: Array[Int] = stage.jobIds.toArray
+ jobsThatUseStage.foreach(jobId => {
+ handleJobCancellation(jobId, "because Stage %s was cancelled".format(stageId))
+ })
--- End diff --
```scala
jobsThatUseStage.foreach { jobId =>
handleJobCancellation(jobId, "because Stage %s was cancelled".format(stageId))
}
```
There are also opportunities throughout to switch from concatenated or formatted strings to string interpolation, which I favor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49970120
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)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331488
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -56,6 +58,16 @@ private[spark] class Stage(
val numPartitions = rdd.partitions.size
val outputLocs = Array.fill[List[MapStatus]](numPartitions)(Nil)
var numAvailableOutputs = 0
+
+ /** List of jobs that this stage belong to. */
--- End diff --
as long as we're nit-ing..."belong" -> "belongs"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50219861
Merging this in master. Thanks for reviewing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331280
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -395,16 +389,9 @@ class DAGScheduler(
activeJobs -= job
if (resultStage.isEmpty) {
- // Clean up result stages.
- val resultStagesForJob = resultStageToJob.keySet.filter(
- stage => resultStageToJob(stage).jobId == job.jobId)
- if (resultStagesForJob.size != 1) {
- logWarning(
- s"${resultStagesForJob.size} result stages for job ${job.jobId} (expect exactly 1)")
- }
- resultStageToJob --= resultStagesForJob
+ job.finalStage.resultOfJob = None
} else {
- resultStageToJob -= resultStage.get
+ resultStage.get.resultOfJob = None
--- End diff --
And as a result of the above, you can consolidate this if/else into one thing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50191092
QA results for PR 1561:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17191/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331172
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -395,16 +389,9 @@ class DAGScheduler(
activeJobs -= job
if (resultStage.isEmpty) {
--- End diff --
Great question...I think we just didn't notice that. I think it's safe to remove it, and you can also remove it as a parameter to failJobAndIndependentStages
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50218632
QA tests have started for PR 1561. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17210/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331519
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -56,6 +58,16 @@ private[spark] class Stage(
val numPartitions = rdd.partitions.size
val outputLocs = Array.fill[List[MapStatus]](numPartitions)(Nil)
var numAvailableOutputs = 0
+
+ /** List of jobs that this stage belong to. */
+ val jobIds = new HashSet[Int]
+ /** For stages that are the final (consists of only ResultTasks), link to the ActiveJob. */
+ var resultOfJob: Option[ActiveJob] = None
+ var pendingTasks: Option[HashSet[Task[_]]] = None
--- End diff --
Why use an option here, as opposed to just a (possibly empty) hash set? The option makes many of the accesses more clumsy.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50209339
QA results for PR 1561:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17199/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49979231
QA tests have started for PR 1561. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17112/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50184542
That makes sense. I was wondering about it myself too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49963642
QA results for PR 1561:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17087/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328329
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -315,13 +309,14 @@ class DAGScheduler(
*/
private def updateJobIdStageIdMaps(jobId: Int, stage: Stage) {
def updateJobIdStageIdMapsList(stages: List[Stage]) {
- if (!stages.isEmpty) {
+ if (stages.nonEmpty) {
val s = stages.head
- stageIdToJobIds.getOrElseUpdate(s.id, new HashSet[Int]()) += jobId
+ s.jobIds += jobId
jobIdToStageIds.getOrElseUpdate(jobId, new HashSet[Int]()) += s.id
- val parents = getParentStages(s.rdd, jobId)
- val parentsWithoutThisJobId = parents.filter(p =>
- !stageIdToJobIds.get(p.id).exists(_.contains(jobId)))
+ val parents: List[Stage] = getParentStages(s.rdd, jobId)
+ val parentsWithoutThisJobId = parents.filter { p: Stage =>
+ !p.jobIds.contains(jobId)
--- End diff --
could now do this as `filter { !_.jobIds.contains(jobId) }`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50209636
hive-thriftserver test failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331310
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -355,14 +351,13 @@ class DAGScheduler(
logDebug("Removing running stage %d".format(stageId))
runningStages -= stage
}
- stageToInfos -= stage
for ((k, v) <- shuffleToMapStage.find(_._2 == stage)) {
shuffleToMapStage.remove(k)
}
- if (pendingTasks.contains(stage) && !pendingTasks(stage).isEmpty) {
+ if (stage.pendingTasks.isDefined && !stage.pendingTasks.isEmpty) {
logDebug("Removing pending status for stage %d".format(stageId))
}
- pendingTasks -= stage
+ stage.pendingTasks = None
--- End diff --
Ok don't worry about it, as discussed. I want to remove this pendingTasks anyway, which is redundant with information stored by the TSM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50209623
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331481
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -22,6 +22,8 @@ import org.apache.spark.rdd.RDD
import org.apache.spark.storage.BlockManagerId
import org.apache.spark.util.CallSite
+import scala.collection.mutable.HashSet
--- End diff --
ha this is WIP.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49971683
Love the cleanup here!!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328234
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -315,13 +309,14 @@ class DAGScheduler(
*/
private def updateJobIdStageIdMaps(jobId: Int, stage: Stage) {
def updateJobIdStageIdMapsList(stages: List[Stage]) {
- if (!stages.isEmpty) {
+ if (stages.nonEmpty) {
val s = stages.head
- stageIdToJobIds.getOrElseUpdate(s.id, new HashSet[Int]()) += jobId
+ s.jobIds += jobId
jobIdToStageIds.getOrElseUpdate(jobId, new HashSet[Int]()) += s.id
- val parents = getParentStages(s.rdd, jobId)
- val parentsWithoutThisJobId = parents.filter(p =>
- !stageIdToJobIds.get(p.id).exists(_.contains(jobId)))
+ val parents: List[Stage] = getParentStages(s.rdd, jobId)
--- End diff --
Why the type annotation?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50203843
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15330894
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -223,7 +217,7 @@ class DAGScheduler(
new Stage(id, rdd, numTasks, shuffleDep, getParentStages(rdd, jobId), jobId, callSite)
stageIdToStage(id) = stage
updateJobIdStageIdMaps(jobId, stage)
- stageToInfos(stage) = StageInfo.fromStage(stage)
+ stage.info = StageInfo.fromStage(stage)
--- End diff --
Why not just do this as part of the Stage constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331465
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -22,6 +22,8 @@ import org.apache.spark.rdd.RDD
import org.apache.spark.storage.BlockManagerId
import org.apache.spark.util.CallSite
+import scala.collection.mutable.HashSet
--- End diff --
Mr. Xin your imports are ordered incorrectly! I expected better of you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49967238
JIRA?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50204787
QA tests have started for PR 1561. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17199/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50184213
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50103068
QA results for PR 1561:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17157/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-49983065
QA results for PR 1561:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17112/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15414988
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -728,8 +697,6 @@ class DAGScheduler(
private def submitMissingTasks(stage: Stage, jobId: Int) {
logDebug("submitMissingTasks(" + stage + ")")
// Get our pending tasks and remember them in our pendingTasks entry
- val myPending = pendingTasks.getOrElseUpdate(stage, new HashSet)
--- End diff --
Actually you should clear it here, I think the issue is if a stage gets resubmitted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15328468
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
logError("No stages registered for job " + job.jobId)
} else {
- stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach {
- case (stageId, jobSet) =>
+ stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {
--- End diff --
Why filter instead of filterKeys? Looks to me like that only serves to make the predicate harder to read.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15383355
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
logError("No stages registered for job " + job.jobId)
} else {
- stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach {
- case (stageId, jobSet) =>
+ stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {
--- End diff --
Actually that's pretty cool. I looked into the source code more. Didn't realize the whole FilteredKeys thing is "lazy".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15356652
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
logError("No stages registered for job " + job.jobId)
} else {
- stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach {
- case (stageId, jobSet) =>
+ stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15331043
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -355,14 +351,13 @@ class DAGScheduler(
logDebug("Removing running stage %d".format(stageId))
runningStages -= stage
}
- stageToInfos -= stage
for ((k, v) <- shuffleToMapStage.find(_._2 == stage)) {
shuffleToMapStage.remove(k)
}
- if (pendingTasks.contains(stage) && !pendingTasks(stage).isEmpty) {
+ if (stage.pendingTasks.isDefined && !stage.pendingTasks.isEmpty) {
logDebug("Removing pending status for stage %d".format(stageId))
}
- pendingTasks -= stage
+ stage.pendingTasks = None
--- End diff --
Is it necessary to clear this? Aren't all references to the Stage removed here anyway? Is the issue that you want this cleared in case the stage is resubmitted?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Removed some HashMaps from DAGScheduler by sto...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15327897
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -56,6 +58,16 @@ private[spark] class Stage(
val numPartitions = rdd.partitions.size
val outputLocs = Array.fill[List[MapStatus]](numPartitions)(Nil)
var numAvailableOutputs = 0
+
+ /** List of jobs that this stage belong to. */
--- End diff --
nit: It's a Set not a List
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/1561#issuecomment-50200148
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
```scala
shuffleToMapStage = new HashMap[Int, Stage]
```
which only holds ShuffleMapStages, so we could give it a more specific type of `HashMap[Int, ShuffleMapStage]`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15332026
--- Diff: core/src/main/scala/org/apache/spark/scheduler/Stage.scala ---
@@ -56,6 +58,16 @@ private[spark] class Stage(
val numPartitions = rdd.partitions.size
val outputLocs = Array.fill[List[MapStatus]](numPartitions)(Nil)
var numAvailableOutputs = 0
+
+ /** List of jobs that this stage belong to. */
+ val jobIds = new HashSet[Int]
+ /** For stages that are the final (consists of only ResultTasks), link to the ActiveJob. */
+ var resultOfJob: Option[ActiveJob] = None
+ var pendingTasks: Option[HashSet[Task[_]]] = None
--- End diff --
And potentially error prone. I don't think there is anyplace left where the distinction between None and Some(emptySet) is useful except for the check in removeStage (https://github.com/rxin/spark/blob/dagSchedulerHashMaps/core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala#L357), and that's not really even useful anymore. On the other hand, having a None where we expect at least Some(emptySet) can be a problem.
I think we're better off without the Option.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Part of [SPARK-2456] Removed some HashMaps fro...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1561#discussion_r15361052
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -341,8 +336,9 @@ class DAGScheduler(
if (registeredStages.isEmpty || registeredStages.get.isEmpty) {
logError("No stages registered for job " + job.jobId)
} else {
- stageIdToJobIds.filterKeys(stageId => registeredStages.get.contains(stageId)).foreach {
- case (stageId, jobSet) =>
+ stageIdToStage.filter(s => registeredStages.get.contains(s._1)).foreach {
--- End diff --
I need the stage object in the foreach after the filter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---