Uh oh!
There was an error while loading. Please reload this page.
[SPARK-27590][CORE] do not consider skipped tasks when scheduling speculative tasks - #24485
[SPARK-27590][CORE] do not consider skipped tasks when scheduling speculative tasks#24485cloud-fan wants to merge 3 commits into
Conversation
cloud-fan
commented
Apr 29, 2019
SparkQA
commented
Apr 29, 2019
Test build #104986 has finished for PR 24485 at commit
|
squito
left a comment
There was a problem hiding this comment.
lgtm, just a small suggestion on a cleanup
| // `successfulTaskDurations` may not equal to `tasksSuccessful`. Here we should only count the | ||
| // tasks that are submitted by this `TaskSetManager` and are completed successfully. | ||
| val numSuccessfulTasks = successfulTaskDurations.size() | ||
| if (numSuccessfulTasks >= minFinishedForSpeculation && numSuccessfulTasks > 0) { |
There was a problem hiding this comment.
unrelated to your change, but a potential cleanup here -- why not compute minFinishedForSpeculation in the construction, and even set it to math.max(1, (speculationQuantile * numTasks).floor.toInt)? Then you also don't need the extra numSuccessfulTasks > 0 here.
jiangxb1987
commented
May 1, 2019
LGTM |
SparkQA
commented
May 6, 2019
Test build #105143 has finished for PR 24485 at commit
|
cloud-fan
commented
May 6, 2019
retest this please |
SparkQA
commented
May 6, 2019
Test build #105150 has finished for PR 24485 at commit
|
| } | ||
| var foundTasks = false | ||
| val minFinishedForSpeculation = (speculationQuantile * numTasks).floor.toInt | ||
| val minFinishedForSpeculation = math.max((speculationQuantile * numTasks).floor.toInt, 1) |
There was a problem hiding this comment.
sorry I had actually meant to just compute this once in the constructor, eg. around line 70
SparkQA
commented
May 6, 2019
Test build #105160 has finished for PR 24485 at commit
|
SparkQA
commented
May 7, 2019
Test build #105186 has finished for PR 24485 at commit
|
squito
commented
May 7, 2019
merged to master. Thanks @cloud-fan ! |
cloud-fan
commented
May 7, 2019
@squito thanks for the review and merge! |
What changes were proposed in this pull request?
This is a followup of #24375
When
TaskSetManagerskips a task because its corresponding partition is already completed by otherTaskSetManagers, we should not consider the duration of the task that is finished by otherTaskSetManagers to schedule the speculative tasks of thisTaskSetManager.How was this patch tested?
updated test case