[GitHub] spark pull request #23106: [SPARK-26141] Enable custom metrics implementatio...

[squito <gi...@git.apache.org>]

Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/23106#discussion_r236418843
  
    --- Diff: core/src/main/java/org/apache/spark/shuffle/sort/ShuffleExternalSorter.java ---
    @@ -242,8 +243,13 @@ private void writeSortedFile(boolean isLastFile) {
           // Note that we intentionally ignore the value of `writeMetricsToUse.shuffleWriteTime()`.
           // Consistent with ExternalSorter, we do not count this IO towards shuffle write time.
           // This means that this IO time is not accounted for anywhere; SPARK-3577 will fix this.
    -      writeMetrics.incRecordsWritten(writeMetricsToUse.recordsWritten());
    -      taskContext.taskMetrics().incDiskBytesSpilled(writeMetricsToUse.bytesWritten());
    +
    +      // This is guaranteed to be a ShuffleWriteMetrics based on the if check in the beginning
    +      // of this file.
    --- End diff --
    
    I found "beginning of this file" to be confusing, I thought you meant the beginning of `ShuffleExternalSorter.java`, not the spill file. maybe "beginning of this method"
    
    also is the comment above this about SPARK-3577 out of date now that has been fixed?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org