You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by LantaoJin <gi...@git.apache.org> on 2018/09/06 14:08:51 UTC
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated metadata in DataSo...
GitHub user LantaoJin opened a pull request:
https://github.com/apache/spark/pull/22353
[SPARK-25357][SQL] Abbreviated metadata in DataSourceScanExec results in incomplete information in event log
## What changes were proposed in this pull request?
Field metadata removed from SparkPlanInfo in SPARK-17701. Corresponding, this field was also removed from event SparkListenerSQLExecutionStart in Spark event log. If we want to analyze event log to get some fields which wider than 100 (e.g the Location or ReadSchema of FileScan), they are abbreviated in simpleString of SparkPlanInfo JSON or physicalPlanDescription JSON.
## How was this patch tested?
Manual test/Check event log
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/LantaoJin/spark SPARK-25357
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22353.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 #22353
----
commit 89fff2ac653fa05ae92ad09ba8307649d6972900
Author: LantaoJin <ji...@...>
Date: 2018-09-06T14:04:56Z
[SPARK-25357][SQL] Abbreviated metadata in DataSourceScanExec results in incomplete information in event log
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated simpleString in DataSourc...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
The purpose is logging meta info like file input file path to event log. So I revert the changes about simpleString and add back the metadata to SparkPlanInfo interface. This change will log metadata into event log in SparkListenerSQLExecutionStart and it is still benefited in #18600 .
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated simpleString in DataSourc...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22353
@LantaoJin . Please check the following example in Spark UI; the hover text on `Scan parquet`.
```scala
scala> spark.range(20000).repartition(10000).write.mode("overwrite").parquet("/tmp/10000")
scala> spark.read.parquet("/tmp/10000/*").count
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated simpleString in Da...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216122273
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala ---
@@ -54,7 +54,7 @@ trait DataSourceScanExec extends LeafExecNode with CodegenSupport {
override def simpleString: String = {
val metadataEntries = metadata.toSeq.sorted.map {
case (key, value) =>
- key + ": " + StringUtils.abbreviate(redact(value), 100)
--- End diff --
Spark users don’t care about it but platform team care. The purpose is to add the missing information back in event log since PR#18600
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo ...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216380605
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -73,6 +73,11 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] with Logging with Serializ
super.makeCopy(newArgs)
}
+ /**
+ * @return Metadata that describes more details of this SparkPlan.
+ */
+ def metadata: Map[String, String] = Map.empty
+
--- End diff --
cc @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated simpleString in Da...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216122062
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala ---
@@ -54,7 +54,7 @@ trait DataSourceScanExec extends LeafExecNode with CodegenSupport {
override def simpleString: String = {
val metadataEntries = metadata.toSeq.sorted.map {
case (key, value) =>
- key + ": " + StringUtils.abbreviate(redact(value), 100)
--- End diff --
We already have `spark.debug.maxToStringFields` for debugging, so is it bad to add `spark.debug.XXXX`? Anyway, if most users don't care about this, I think changing the default number goes too far.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
Spark driver log is always distributed on various client nodes and depends on the log4j configs. In a big company, it's hard to collect them all and I think it's better to used for debug not analyze.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated simpleString in Da...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216134032
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala ---
@@ -54,7 +54,7 @@ trait DataSourceScanExec extends LeafExecNode with CodegenSupport {
override def simpleString: String = {
val metadataEntries = metadata.toSeq.sorted.map {
case (key, value) =>
- key + ": " + StringUtils.abbreviate(redact(value), 100)
--- End diff --
This seems to cause a regression on Spark Web UI. Could you check that, @LantaoJin ?
In fact, the abbreviation is introduced over two years ago at Spark 2.0 intentionally for UI via [[SPARK-14476][SQL] Improve the physical plan visualization by adding meta info like table name and file path for data source](https://github.com/apache/spark/pull/12947). At least, we had better update the information of PR and JIRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22353
**[Test build #95993 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95993/testReport)** for PR 22353 at commit [`0340fa6`](https://github.com/apache/spark/commit/0340fa648a17384a039ee484de9ce91a0129b260).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated simpleString in Da...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216121128
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala ---
@@ -54,7 +54,7 @@ trait DataSourceScanExec extends LeafExecNode with CodegenSupport {
override def simpleString: String = {
val metadataEntries = metadata.toSeq.sorted.map {
case (key, value) =>
- key + ": " + StringUtils.abbreviate(redact(value), 100)
--- End diff --
I think it’s overkill to parameterizing it. And Spark user doesn’t care about it, no one will reset it before submitting app. Besides, simply raise up to 1000 also can resolve the problem on most cases, but longer than 1000 chars is still meanlessness.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated simpleString in Da...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216122293
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala ---
@@ -54,7 +54,7 @@ trait DataSourceScanExec extends LeafExecNode with CodegenSupport {
override def simpleString: String = {
val metadataEntries = metadata.toSeq.sorted.map {
case (key, value) =>
- key + ": " + StringUtils.abbreviate(redact(value), 100)
--- End diff --
It’s not for debugging, it’s for offline analysis based on event log.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22353
So you need a way to reliably report some extra information like file path in the event logs, but don't want to show it in the UI as it maybe too long.
Basically we shouldn't put such information in the event logs if it's not used in the UI, and we should build a new mechanism to make Spark easier to be analyzed. Also keep it mind that event logs are not reliable, Spark may drop some events if the event bus is too busy.
I'm ok to add it back to the event logs since it was there before, but please don't add `metadata` to `SparkPlan`, we can pattern match the `FileSourceScanExec` in `SparkPlanInfo.fromSparkPlan`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22353
Although event log is in JSON format, it's mostly for internal usage, to be load by history server and used to build the Spark UI. For compatibility, we only focus on making history to be able to load event logs from different spark versions, not the event log itself. At the end it's still a log.
Metadata is a hack which I really hate to add back. Can you describe more details about your use case? Let's see if we can solve it with the Spark driver log.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22353
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
ping @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo ...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r217165648
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanInfo.scala ---
@@ -59,6 +57,12 @@ private[execution] object SparkPlanInfo {
new SQLMetricInfo(metric.name.getOrElse(key), metric.id, metric.metricType)
}
- new SparkPlanInfo(plan.nodeName, plan.simpleString, children.map(fromSparkPlan), metrics)
+ // dump the file scan metadata (e.g file path) to event log
--- End diff --
As a next step of reviews, did you have a chance to test this on your real environment at least TPCDS 1TB?
This seems to increase the event log traffic dramatically in the worst case. Can we have some comparison before and after this PR? @LantaoJin .
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated simpleString in DataSourc...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
Thanks @dongjoon-hyun . That would be a problem. Seems setting to 200 or 500 are cause a limited regression on hover text.
Hard code to 500 shows:
<img width="215" alt="screen shot 2018-09-10 at 5 03 53 pm" src="https://user-images.githubusercontent.com/1853780/45287713-8c74a800-b51b-11e8-94aa-9d3f0cd36f28.png">
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated simpleString in DataSourc...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
A scenario here is after an application completed, there is no way to know the intact file path of File Scan Exec if the path width is longer than 100 chars.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22353
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated metadata in DataSourceSca...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22353
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Abbreviated simpleString in Da...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r216119211
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala ---
@@ -54,7 +54,7 @@ trait DataSourceScanExec extends LeafExecNode with CodegenSupport {
override def simpleString: String = {
val metadataEntries = metadata.toSeq.sorted.map {
case (key, value) =>
- key + ": " + StringUtils.abbreviate(redact(value), 100)
--- End diff --
How about parameterizing the second value as a new `SQLConf`? Or, defining `verboseString`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated simpleString in DataSourc...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
@wangyum @hvanhovell
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22353
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
Thank you @cloud-fan for your reminding. We’ve handled the drop message case. Agree, I will update a commit tomorrow.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated metadata in DataSourceSca...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22353
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated metadata in DataSourceSca...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22353
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Abbreviated metadata in DataSourceSca...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
@cloud-fan @gatorsmile @dongjoon-hyun , kindly help to review.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo ...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22353#discussion_r217229063
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanInfo.scala ---
@@ -59,6 +57,12 @@ private[execution] object SparkPlanInfo {
new SQLMetricInfo(metric.name.getOrElse(key), metric.id, metric.metricType)
}
- new SparkPlanInfo(plan.nodeName, plan.simpleString, children.map(fromSparkPlan), metrics)
+ // dump the file scan metadata (e.g file path) to event log
--- End diff --
Not yet. This field only removed from 2.3. The event log size should be same with before 2.3. The main increasing is input path. The example above read 10000 parquet files will log 10000 paths in one SQLExecutionStart event. No regression pathes. The size increased is foreseeable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22353
**[Test build #95993 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95993/testReport)** for PR 22353 at commit [`0340fa6`](https://github.com/apache/spark/commit/0340fa648a17384a039ee484de9ce91a0129b260).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22353
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95993/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22353
> Although event log is in JSON format, it's mostly for internal usage, to be load by history server and used to build the Spark UI.
AFAIK, there are more and more projects replay event log to analysis jobs offline, especially in a platform/infra team in a big company. Dr-elephant doesn't read event log, instead, query SHS to get information causing many problems like compatibility or data accuracy. In eBay we are building a system similar with Dr-elephant but much powerful. One of use cases in this system is building a data lineage and monitor the input/output path and data size for each application. Difference with Apache Altas who need attach a spark listener into the spark runtime, we choose to replay event log to build all context we need. Before 2.3, we can get above information from the `metadata` field in SQLExecutionStart event. Now it was removed. So I hope this PR could add it back. What's more is make more probability on event log instead of only using in SHS.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22353: [SPARK-25357][SQL] Add metadata to SparkPlanInfo to dump...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22353
thanks, merging to master/2.4/2.3!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org