[SPARK-3465] fix task metrics aggregation in local mode #2338
Conversation
|
cc @sryza, is there a better way to fix it? |
|
Hi @davies , sorry for causing this bug and thanks for picking it up. To avoid making the deep copy unnecessarily when running in non-local mode, we could instead make it on the executor side, and only do so if isLocal = true. Any issues you can see with that? |
|
@sryza I had changed it to do the copy in Executor, then it's hard to write a test now. Any idea? |
|
QA tests have started for PR 2338 at commit
|
|
Tests timed out after a configured wait of |
|
QA tests have started for PR 2338 at commit
|
|
QA tests have finished for PR 2338 at commit
|
|
QA tests have started for PR 2338 at commit
|
|
QA tests have finished for PR 2338 at commit
|
|
LGTM |
| @@ -360,7 +360,13 @@ private[spark] class Executor( | |||
| if (!taskRunner.attemptedTask.isEmpty) { | |||
| Option(taskRunner.task).flatMap(_.metrics).foreach { metrics => | |||
| metrics.updateShuffleReadMetrics | |||
| tasksMetrics += ((taskRunner.taskId, metrics)) | |||
| if (isLocal) { | |||
| // make a deep copy of it | |||
There was a problem hiding this comment.
Could you elaborate in the comment more specifically why we need to do this?
|
I don't have any great ideas for how to write a test for it, but this looks good to me as well. |
|
@andrewor14 @sryza done |
|
QA tests have started for PR 2338 at commit
|
|
QA tests have finished for PR 2338 at commit
|
|
Thanks @davies. This is going into master and 1.1. |
Before overwrite t.taskMetrics, take a deepcopy of it. Author: Davies Liu <[email protected]> Closes #2338 from davies/fix_metric and squashes the following commits: a5cdb63 [Davies Liu] Merge branch 'master' into fix_metric 7c879e0 [Davies Liu] add more comments 754b5b8 [Davies Liu] copy taskMetrics only when isLocal is true 5ca26dc [Davies Liu] fix task metrics aggregation in local mode
Before overwrite t.taskMetrics, take a deepcopy of it.