[SPARK-19793] Use clock.getTimeMillis when mark task as finished in TaskSetManager. #17133
Conversation
|
Test build #73749 has finished for PR 17133 at commit
|
|
CC @devaraj-kavali and possibly @zsxwing ? |
vanzin
left a comment
vanzin
left a comment
There was a problem hiding this comment.
Other than the cryptic comments, looks ok to me.
| val info = taskInfos(tid) | ||
| val index = info.index | ||
| info.markFinished(TaskState.FINISHED) | ||
| // Mark task as finished. Note that finishing time here should be bigger than 0. |
There was a problem hiding this comment.
I'm not sure what this comment is trying to say, especially since there's no check anywhere for "finishing time > 0" in the code here.
Also, grammar nit: it should be "larger than 0".
There was a problem hiding this comment.
Thanks for review :) what I want to say is that finishTime of TaskInfo should be set larger than 0(when do unit test), otherwise TaskInfo.finishTime will return false. I removed the comments and added an assert inside TaskInfo.
|
Test build #73795 has finished for PR 17133 at commit
|
|
Test build #73807 has finished for PR 17133 at commit
|
|
|
||
| private[spark] def markFinished(state: TaskState, time: Long = System.currentTimeMillis) { | ||
| // finishTime should be set larger than 0, otherwise "finished" below will return false. | ||
| assert(time != 0) |
There was a problem hiding this comment.
Nit: shouldn't you then assert that time > 0 given the comment? do you really mean to assert this, or is it an argument check?
There was a problem hiding this comment.
Yes, I want to assert that time>0
|
Test build #73839 has finished for PR 17133 at commit
|
|
Test build #73849 has finished for PR 17133 at commit
|
| gettingResultTime = time | ||
| } | ||
|
|
||
| private[spark] def markFinished(state: TaskState, time: Long = System.currentTimeMillis) { |
There was a problem hiding this comment.
How about removing the default value for the argument? I only see one other call site for this method that would need to be changed.
|
Test build #73881 has finished for PR 17133 at commit
|
|
Merging to master. |
4 participants
What changes were proposed in this pull request?
TaskSetManager is now using
System.getCurrentTimeMilliswhen mark task as finished inhandleSuccessfulTaskandhandleFailedTask. Thus developer cannot set the tasks finishing time in unit test. WhenhandleSuccessfulTask, task's duration =System.getCurrentTimeMillis- launchTime(which can be set byclock), the result is not correct.How was this patch tested?
Existing tests.