[SPARK-23326][WEBUI]schedulerDelay should return 0 when the task is running #20493
Conversation
|
Test build #87015 has finished for PR 20493 at commit
|
| class AppStatusUtilsSuite extends SparkFunSuite { | ||
|
|
||
| test("schedulerDelay") { | ||
| val runningTask = new TaskData( |
There was a problem hiding this comment.
Can we make this test case more concise and easy to read by deduplication?
For the purpose of this test case, what about the following pattern?
Seq(("RUNNING", 0), ("SUCCESS", 3L)).foreach { case (status, schedulerDelay) =>
// the code from `finishedTask`
}
There was a problem hiding this comment.
Actually there are many different values between these 2 code blocks
+ executorDeserializeTime = 5L,
+ executorDeserializeCpuTime = 3L,
+ executorRunTime = 90L,
+ executorCpuTime = 10L,
+ resultSize = 100L,
+ jvmGcTime = 10L,
+ resultSerializationTime = 2L,
I think it's OK keep the code as it is.
There was a problem hiding this comment.
Yeah, I'm inclined to keep it as they are more real.
| def schedulerDelay(task: TaskData): Long = { | ||
| if (task.taskMetrics.isDefined && task.duration.isDefined) { | ||
| if (isTaskFinished(task) && task.taskMetrics.isDefined && task.duration.isDefined) { |
There was a problem hiding this comment.
task.duration.isDefined should be redundant now, right?
(I remember the duration didn't use to be set for running tasks, so this code worked, but apparently it changed while I worked on these changes...)
There was a problem hiding this comment.
Logically duration should be set for running tasks, to indicate how long a task has been run.
I feel it's safer to keep task.duration.isDefined, as we call task.duration.get below.
|
LGTM |
|
thanks, merging to master/2.3! |
…running ## What changes were proposed in this pull request? When a task is still running, metrics like executorRunTime are not available. Then `schedulerDelay` will be almost the same as `duration` and that's confusing. This PR makes `schedulerDelay` return 0 when the task is running which is the same behavior as 2.2. ## How was this patch tested? `AppStatusUtilsSuite.schedulerDelay` Author: Shixiong Zhu <[email protected]> Closes #20493 from zsxwing/SPARK-23326. (cherry picked from commit f3f1e14) Signed-off-by: Wenchen Fan <[email protected]>
What changes were proposed in this pull request?
When a task is still running, metrics like executorRunTime are not available. Then
schedulerDelaywill be almost the same asdurationand that's confusing.This PR makes
schedulerDelayreturn 0 when the task is running which is the same behavior as 2.2.How was this patch tested?
AppStatusUtilsSuite.schedulerDelay