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

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/23106


---

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

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

[rxin]

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

https://github.com/apache/spark/pull/23106#discussion_r236432889
  
--- 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 --

ah yes. nice catch


---

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

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

[squito]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org