[SPARK-16827] Stop reporting spill metrics as shuffle metrics #15347
Conversation
|
Can you also work on reporting spill metrics? |
|
Sure, having the actual spill metrics is something we're interested in as well. I'd like to work on it, but I might not get to it immediately. |
|
Jenkins, add to whitelist. |
|
Test build #3296 has finished for PR 15347 at commit
|
| this.writeMetrics = taskContext.taskMetrics().shuffleWriteMetrics(); | ||
| // The spill metrics are stored in a new ShuffleWriteMetrics, and then discarded (this fixes SPARK-16827). | ||
| // TODO: Instead, separate spill metrics should be stored and reported (tracked in SPARK-3577). | ||
| this.writeMetrics = new ShuffleWriteMetrics(); |
There was a problem hiding this comment.
so this isn't actually used anywhere right now? Is that what the TODO is about?
There was a problem hiding this comment.
Yes, that's correct.
|
OK, this change by itself LGTM. @dafrista would you mind creating a separate JIRA (or point me to an existing one) about the TODO then? Merging this into master |
|
@andrewor14 sure I think the one mentioned in the TODO covers it (SPARK-3577) |
|
@dafrista can you create a backport for branch-2.0? |
|
@rxin , "Can you also work on reporting spill metrics?" --> here , you mean reporting spill time metrics , right ? Since I think write spill bytes is already included - https://github.com/facebook/FB-Spark/blob/fb-2.0/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java#L232 |
…trics ## What changes were proposed in this pull request? Fix a bug where spill metrics were being reported as shuffle metrics. Eventually these spill metrics should be reported (SPARK-3577), but separate from shuffle metrics. The fix itself basically reverts the line to what it was in 1.6. ## How was this patch tested? Cherry-picked from master (#15347) Author: Brian Cho <bcho@fb.com> Closes #15455 from dafrista/shuffle-metrics-2.0.
## What changes were proposed in this pull request? Fix a bug where spill metrics were being reported as shuffle metrics. Eventually these spill metrics should be reported (SPARK-3577), but separate from shuffle metrics. The fix itself basically reverts the line to what it was in 1.6. ## How was this patch tested? Tested on a job that was reporting shuffle writes even for the final stage, when no shuffle writes should take place. After the change the job no longer shows these writes. Before:  After: <img width="1052" alt="screen shot 2016-10-03 at 11 44 44 pm" src="https://cloud.githubusercontent.com/assets/1514239/19085903/e173a860-8a20-11e6-85e3-d47f9835f494.png"> Author: Brian Cho <bcho@fb.com> Closes apache#15347 from dafrista/shuffle-metrics.
What changes were proposed in this pull request?
Fix a bug where spill metrics were being reported as shuffle metrics. Eventually these spill metrics should be reported (SPARK-3577), but separate from shuffle metrics. The fix itself basically reverts the line to what it was in 1.6.
How was this patch tested?
Tested on a job that was reporting shuffle writes even for the final stage, when no shuffle writes should take place. After the change the job no longer shows these writes.
Before:
After:
