[SPARK-26142][SQL] Implement shuffle read metrics in SQL #23128

xuanyuanking · 2018-11-23T21:23:02Z

What changes were proposed in this pull request?

Implement SQLShuffleMetricsReporter on the sql side as the customized ShuffleMetricsReporter, which extended the TempShuffleReadMetrics and update SQLMetrics, in this way shuffle metrics can be reported in the SQL UI.

How was this patch tested?

Add UT in SQLMetricsSuite.
Manual test locally, before:

after:

gatorsmile · 2018-11-23T22:16:36Z

@xuanyuanking Could you address the conflicts? Thanks for you fast work!

rxin · 2018-11-23T23:19:55Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLShuffleMetricsReporter.scala

Doing a hashmap lookup here could introduce serious performance regressions.

(I’m not referring to just this function, but in general, especially for per-row).

Sorry for the less consideration on per-row operation here, I should be more careful. Fix done in cb46bfe.

SparkQA · 2018-11-24T00:54:35Z

Test build #99220 has finished for PR 23128 at commit 1b556ec.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SQLShuffleMetricsReporter(

xuanyuanking · 2018-11-24T05:35:44Z

@gatorsmile Thanks Xiao! Conflicts resolve done, as Reynold comments in #23105 (comment), when the ShuffleMetricsReporter move to ShuffleReadMetricsReporter in write pr, it will conflict again here, I'll keep tracking the relevant pr.

SparkQA · 2018-11-24T08:05:02Z

Test build #99223 has finished for PR 23128 at commit cb46bfe.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2018-11-24T08:41:46Z

retest this please.

SparkQA · 2018-11-24T11:17:05Z

Test build #99225 has finished for PR 23128 at commit cb46bfe.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-24T21:43:01Z

Test build #4440 has finished for PR 23128 at commit cb46bfe.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-11-25T17:30:24Z

Test build #99236 has finished for PR 23128 at commit 8689acb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala

cloud-fan · 2018-11-27T07:07:52Z

retest this please

SparkQA · 2018-11-27T08:05:01Z

Test build #99308 has finished for PR 23128 at commit 8689acb.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-11-27T08:22:48Z

retest this please

SparkQA · 2018-11-27T11:55:53Z

Test build #99311 has finished for PR 23128 at commit 8689acb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-11-27T12:14:22Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ShuffledRowRDD.scala

I don't think we need to consider this case since ShuffledRowRDD is a private API. If we do need to consider it, we also need to take care if users pass in a metrics that is invalid.

do you mean we may leave the metrics empty when creating ShuffledRowRDD in tests?

do you mean we may leave the metrics empty when creating ShuffledRowRDD in tests?

Yes, like we did in UnsafeRowSerializerSuite.

I don't think we need to consider this case since ShuffledRowRDD is a private API

Got it, after search new ShuffledRowRDD in all source code, UnsafeRowSerializerSuite is the only place, I'll change the test and delete the default value of metrics in this commit.

Done in 0348ae5.

cloud-fan · 2018-11-27T12:18:26Z

LGTM except one comment

SparkQA · 2018-11-27T19:08:05Z

Test build #99334 has finished for PR 23128 at commit 0348ae5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/main/scala/org/apache/spark/sql/execution/ShuffledRowRDD.scala

SparkQA · 2018-11-28T06:05:27Z

Test build #99347 has finished for PR 23128 at commit d12ea31.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2018-11-28T06:42:21Z

python UT failed cause jvm crush.

xuanyuanking · 2018-11-28T07:14:17Z

retest this please

SparkQA · 2018-11-28T08:05:02Z

Test build #99355 has finished for PR 23128 at commit d12ea31.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-11-28T08:09:13Z

retest this please

cloud-fan · 2018-11-28T08:12:28Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ShuffledRowRDD.scala

-    val metrics = context.taskMetrics().createTempShuffleReadMetrics()
+    val tempMetrics = context.taskMetrics().createTempShuffleReadMetrics()
+    // Wrap the tempMetrics with SQLShuffleMetricsReporter here to support
+    // shuffle read metrics in SQL.


// `SQLShuffleMetricsReporter` will update its own metrics for SQL exchange operator, // as well as the `tempMetrics` for basic shuffle metrics.

Thanks Wenchen, done in 8e84c5b.

SparkQA · 2018-11-28T11:40:02Z

Test build #99359 has finished for PR 23128 at commit d12ea31.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-28T11:51:43Z

Test build #99363 has finished for PR 23128 at commit 8e84c5b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-11-28T12:18:24Z

thanks , merging to master!

xuanyuanking · 2018-11-28T12:33:28Z

Thanks @cloud-fan @gatorsmile @rxin !

rxin · 2018-11-28T15:22:19Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala

+  /**
+   * Create all shuffle read relative metrics and return the Map.
+   */
+  def getShuffleReadMetrics(sc: SparkContext): Map[String, SQLMetric] = Map(


I'd prefer to name this create, rather than get, to imply we are creating a new set rather than just returning some existing sets.

Thanks, rename it to createShuffleReadMetrics and move to SQLShuffleMetricsReporter. Done in #23175.

rxin · 2018-11-28T15:22:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLShuffleMetricsReporter.scala

+ *   contains all shuffle metrics defined in [[SQLMetrics.getShuffleReadMetrics]].
+ */
+private[spark] class SQLShuffleMetricsReporter(
+  tempMetrics: TempShuffleReadMetrics,


4 space indent

Thanks ,done in #23175.

rxin · 2018-11-28T15:23:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala


  private val baseForAvgMetric: Int = 10

+  val REMOTE_BLOCKS_FETCHED = "remoteBlocksFetched"


rather than putting this list and the getShuffleReadMetrics function here, we should move it into SQLShuffleMetricsReporter. Otherwise in the future when one adds another metric, he/she is likely to forget to update SQLShuffleMetricsReporter.

rxin · 2018-11-28T15:25:08Z

@xuanyuanking @cloud-fan when you think about where to put each code block, make sure you also think about future evolution of the codebase. In general put relevant things closer to each other (e.g. in one class, one file, or one method).

xuanyuanking · 2018-11-29T02:49:42Z

@rxin Thanks for guidance, I'll address these comments in a follow up PR soon.

## What changes were proposed in this pull request? Implement `SQLShuffleMetricsReporter` on the sql side as the customized ShuffleMetricsReporter, which extended the `TempShuffleReadMetrics` and update SQLMetrics, in this way shuffle metrics can be reported in the SQL UI. ## How was this patch tested? Add UT in SQLMetricsSuite. Manual test locally, before: ![image](https://user-images.githubusercontent.com/4833765/48960517-30f97880-efa8-11e8-982c-92d05938fd1d.png) after: ![image](https://user-images.githubusercontent.com/4833765/48960587-b54bfb80-efa8-11e8-8e95-7a3c8c74cc5c.png) Closes apache#23128 from xuanyuanking/SPARK-26142. Lead-authored-by: Yuanjian Li <[email protected]> Co-authored-by: liyuanjian <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…LShuffleMetricsReporter ## What changes were proposed in this pull request? Follow up for apache#23128, move sql read metrics relatives to `SQLShuffleMetricsReporter`, in order to put sql shuffle read metrics relatives closer and avoid possible problem about forgetting update SQLShuffleMetricsReporter while new metrics added by others. ## How was this patch tested? Existing tests. Closes apache#23175 from xuanyuanking/SPARK-26142-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Reynold Xin <[email protected]>

xuanyuanking changed the title ~~[SPARK-26139][SQL] Support passing shuffle metrics to exchange operator~~ [SPARK-26142][SQL] Support passing shuffle metrics to exchange operator Nov 23, 2018

rxin reviewed Nov 23, 2018

View reviewed changes

xuanyuanking force-pushed the SPARK-26142 branch from 1b556ec to cb46bfe Compare November 24, 2018 05:28

xuanyuanking mentioned this pull request Nov 24, 2018

[SPARK-26140] Enable custom metrics implementation in shuffle reader #23105

Closed

xuanyuanking force-pushed the SPARK-26142 branch from cb46bfe to 8689acb Compare November 25, 2018 13:51

cloud-fan reviewed Nov 26, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 27, 2018

View reviewed changes

liyuanjian and others added 3 commits November 27, 2018 23:38

[SPARK-26142] Implement shuffle read metric in SQL

b46dafc

Fix performance regression and more comments

6ad4657

Address comments

0348ae5

xuanyuanking force-pushed the SPARK-26142 branch from 8689acb to 0348ae5 Compare November 27, 2018 15:39

xuanyuanking changed the title ~~[SPARK-26142][SQL] Support passing shuffle metrics to exchange operator~~ [SPARK-26142][SQL] Implement shuffle read metrics in SQL Nov 28, 2018

xuanyuanking commented Nov 28, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/ShuffledRowRDD.scala Outdated Show resolved Hide resolved

modify comments

d12ea31

cloud-fan reviewed Nov 28, 2018

View reviewed changes

cloud-fan approved these changes Nov 28, 2018

View reviewed changes

update comment

8e84c5b

asfgit closed this in 93112e6 Nov 28, 2018

xuanyuanking deleted the SPARK-26142 branch November 28, 2018 12:33

rxin reviewed Nov 28, 2018

View reviewed changes

xuanyuanking mentioned this pull request Nov 29, 2018

[SPARK-26142]followup: Move sql shuffle read metrics relatives to SQLShuffleMetricsReporter #23175

Closed


		private val baseForAvgMetric: Int = 10

		val REMOTE_BLOCKS_FETCHED = "remoteBlocksFetched"

[SPARK-26142][SQL] Implement shuffle read metrics in SQL #23128

[SPARK-26142][SQL] Implement shuffle read metrics in SQL #23128

Uh oh!

Conversation

xuanyuanking commented Nov 23, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented Nov 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 24, 2018

Uh oh!

xuanyuanking commented Nov 24, 2018

Uh oh!

SparkQA commented Nov 24, 2018

Uh oh!

xuanyuanking commented Nov 24, 2018

Uh oh!

SparkQA commented Nov 24, 2018

Uh oh!

SparkQA commented Nov 24, 2018

Uh oh!

SparkQA commented Nov 25, 2018

Uh oh!

Uh oh!

cloud-fan commented Nov 27, 2018

Uh oh!

SparkQA commented Nov 27, 2018

Uh oh!

cloud-fan commented Nov 27, 2018

Uh oh!

SparkQA commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 27, 2018

Uh oh!

SparkQA commented Nov 27, 2018

Uh oh!

Uh oh!

SparkQA commented Nov 28, 2018

Uh oh!

xuanyuanking commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuanyuanking commented Nov 28, 2018

Uh oh!

SparkQA commented Nov 28, 2018

Uh oh!

cloud-fan commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 28, 2018

Uh oh!

SparkQA commented Nov 28, 2018

Uh oh!

cloud-fan commented Nov 28, 2018

Uh oh!

xuanyuanking commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuanyuanking Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

xuanyuanking commented Nov 28, 2018 •

edited

Loading

xuanyuanking Nov 29, 2018 •

edited

Loading