You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by andrewor14 <gi...@git.apache.org> on 2016/04/15 23:26:26 UTC
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
GitHub user andrewor14 opened a pull request:
https://github.com/apache/spark/pull/12427
[SPARK-14670][SQL] Allow updating SQLMetrics on driver
## What changes were proposed in this pull request?
On the SparkUI right now we have this SQLTab that displays accumulator values per operator. However, it only displays metrics updated on the executors, not on the driver. It is useful to also include driver metrics, e.g. broadcast time.
After this patch you can update metrics defined in `SparkPlan.metrics` in `doConsume` (if whole stage codegen is enabled) or `doExecute`.
## How was this patch tested?
`SQLListenerSuite`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andrewor14/spark sql-metrics-driver
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12427.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #12427
----
commit 3f57261071195970f5e35797cec81192022d82c5
Author: Andrew Or <an...@databricks.com>
Date: 2016-04-15T20:24:06Z
Support updating SQL metrics on driver
commit 1cd3995386ced6e53308ba288942f806183b9eda
Author: Andrew Or <an...@databricks.com>
Date: 2016-04-15T21:23:49Z
Add test
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-217064757
I have the same concern with @zsxwing , what if we execute a DataFrame multiple times?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-210652167
@davies @zsxwing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-219498948
I won't have time to update this in the this week. I can re-open it next week but if one of you want to take it over before that feel free to do so.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/12427#discussion_r60626984
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala ---
@@ -38,7 +38,13 @@ case class SparkListenerSQLExecutionStart(
extends SparkListenerEvent
@DeveloperApi
-case class SparkListenerSQLExecutionEnd(executionId: Long, time: Long)
+case class SparkListenerSQLExecutionEnd(
+ executionId: Long,
+ time: Long,
+ // This allows the driver to update an accumulator's value and have it reported
+ // on the SQL UI. The accumulator needs to be part of SparkPlan.metrics and updated
+ // in doExecute or doConsume, depending on whether whole stage codegen is enabled.
+ driverAccumUpdates: Seq[AccumulableInfo] = Seq.empty[AccumulableInfo])
--- End diff --
As this class uses the default parameter, SparkListenerSQLExecutionEnd only has one constructor: `this(executionId, time, driverAccumUpdates)`. Recovering from old JSON string will set `driverAccumUpdates` to `null`.
Hence, we cannot use the default parameter here. Instead, we should add two constructors manually like this:
```Scala
@DeveloperApi
case class SparkListenerSQLExecutionEnd(
executionId: Long,
time: Long,
// This allows the driver to update an accumulator's value and have it reported
// on the SQL UI. The accumulator needs to be part of SparkPlan.metrics and updated
// in doExecute or doConsume, depending on whether whole stage codegen is enabled.
driverAccumUpdates: Seq[AccumulableInfo])
extends SparkListenerEvent {
def this(executionId: Long, time: Long) {
this(executionId, time, Seq.empty)
}
}
object SparkListenerSQLExecutionEnd {
def apply(executionId: Long, time: Long): SparkListenerSQLExecutionEnd = {
new SparkListenerSQLExecutionEnd(executionId, time)
}
}
```
Could you also add a test like this to make sure we don't break the compatibility?
```Scala
val event = SparkListenerSQLExecutionEnd(1L, 123L)
val json = JsonProtocol.sparkEventToJson(event)
val oldJson = json.removeField({ _._1 == "driverAccumUpdates" })
assert(event === JsonProtocol.sparkEventFromJson(oldJson))
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-217002753
@andrewor14 @cloud-fan Can we target this one to 2.0? The metrics of BroadcastExchange depends on this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-210670983
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/55963/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 closed the pull request at:
https://github.com/apache/spark/pull/12427
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-213041496
What will happen if reusing a DataFrame? `localValue` in the driver will be a sum of all jobs if a DataFrame?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-210652684
**[Test build #55963 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55963/consoleFull)** for PR 12427 at commit [`1cd3995`](https://github.com/apache/spark/commit/1cd3995386ced6e53308ba288942f806183b9eda).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/12427#discussion_r59962714
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala ---
@@ -52,8 +52,14 @@ private[sql] object SQLExecution {
try {
body
} finally {
+ val driverAccumUpdates = queryExecution.executedPlan
+ .collect { case plan: SparkPlan => plan }
+ .flatMap(_.metrics.values.toSeq)
+ .map { a => a.toInfo(Some(a.localValue), None) }
--- End diff --
Should we filter out the metrics that have not be updated (zero value)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-210670981
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/12427#discussion_r60521138
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala ---
@@ -52,8 +52,14 @@ private[sql] object SQLExecution {
try {
body
} finally {
+ val driverAccumUpdates = queryExecution.executedPlan
+ .collect { case plan: SparkPlan => plan }
+ .flatMap(_.metrics.values.toSeq)
+ .map { a => a.toInfo(Some(a.localValue), None) }
--- End diff --
ping
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-210670741
**[Test build #55963 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55963/consoleFull)** for PR 12427 at commit [`1cd3995`](https://github.com/apache/spark/commit/1cd3995386ced6e53308ba288942f806183b9eda).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14670][SQL] Allow updating SQLMetrics o...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/12427#issuecomment-217004438
OK, I'll bring this up to date
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org