[SPARK-27164][Core] RDD.countApprox on empty RDDs schedules jobs which never complete#24100
[SPARK-27164][Core] RDD.countApprox on empty RDDs schedules jobs which never complete#24100ajithme wants to merge 6 commits intoapache:masterfrom
Conversation
|
ok to test |
|
CC @squito @jinxing64 for a similar change earlier. It looks plausible but I'm not sure if it's the best way to do it in this code or not. |
| @@ -1239,6 +1239,9 @@ private[spark] class DAGScheduler( | |||
| markMapStageJobsAsFinished(stage) | |||
| case stage : ResultStage => | |||
| logDebug(s"Stage ${stage} is actually done; (partitions: ${stage.numPartitions})") | |||
There was a problem hiding this comment.
would this happen for normal jobs?
There was a problem hiding this comment.
This condition is guarded by a check when ResultStage itself has zero tasks to run. So it would be skipped for normal jobs
There was a problem hiding this comment.
I mean, does RDD#count have the same bug when rdd is empty? I think it doesn't but I'm trying to understand why it's an issue only for approximate jobs.
There was a problem hiding this comment.
I think this is related to SPARK-26714 / #23637 -- maybe the same change from DAGScheduler.submitJob is needed in DAGScheduler.runApproximateJob
There was a problem hiding this comment.
@cloud-fan, RDD#count scenario is handled via submitJob as initially we have a partitions.size==0 check, just like @squito mentioned
@squito so do you suggest i provide a similar check in runApproximateJob.?? shall i change this PR.?
There was a problem hiding this comment.
I think @squito suggestion is better, i have changed my PR as per it. Please review
|
Test build #103532 has finished for PR 24100 at commit
|
|
Test build #103567 has finished for PR 24100 at commit
|
|
Merged to master |
What changes were proposed in this pull request?
When Result stage has zero tasks, the Job End event is never fired, hence the Job is always running in UI. Example: sc.emptyRDD[Int].countApprox(1000) never finishes even it has no tasks to launch
How was this patch tested?
Added UT