[SPARK-40834][SQL] Use SparkListenerSQLExecutionEnd to track final SQL status in UI #38302

ulysses-you · 2022-10-19T01:43:39Z

What changes were proposed in this pull request?

Use event SparkListenerSQLExecutionEnd to track if the SQL/DataFrame is completion instead of using job status.

Why are the changes needed?

The SQL may succeed with some failed jobs. For example, a inner join with one empty side and one large side, the plan would finish and the large side is still running.

Does this PR introduce any user-facing change?

yes, correct the sql status in UI

How was this patch tested?

add test for backward compatibility and manually test

CREATE TABLE t1 (c1 int) USING PARQUET;
CREATE TABLE t2 USING PARQUET AS SELECT 1 AS c2;

./bin/spark-sql -e "SELECT /*+ merge(tmp) */ * FROM t1 JOIN (SELECT c2, java_method('java.lang.Thread', 'sleep', 10000L) FROM t2) tmp ON c1 = c2;"

before:

after:

ulysses-you · 2022-10-19T01:45:25Z

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

not sure if we need to expose the full error stack

how do we report error message before this PR?

two use cases:

live event listener
User can get the original exception from QueryExecutionListener

history event
User can not get exception from SparkListenerSQLExecutionEnd. In general, the error info can be catched from stage events

why do we need to add error to SparkListenerSQLExecutionEnd now?

To clarify, I think adding a isFailed flag should be sufficient, but since we need to change the event, it's better to add error message which is better. Do I understand it correctly?

yes, right. As we are here, I think error msg has more meaningful than a flag, both of them can work though.

shall we call SparkThrowableHelper.getMessage so that users can get the JSON format error message via config? cc @MaxGekk

or always use JSON format as this will be stored in the event log.

here we catch all Throwable, how about this ?

ex.map { case e: SparkThrowable => SparkThrowableHelper.getMessage(e, ErrorMessageFormat.STANDARD) case e => // unexpected behavior s"${e.getClass.getCanonicalName}: ${e.getMessage}" })

Can we make s"${e.getClass.getCanonicalName}: ${e.getMessage}" JSON format as well? e.g. the java class name can be the error class.

ulysses-you · 2022-10-19T01:45:42Z

cc @gengliangwang @cloud-fan @HyukjinKwon thank you

ulysses-you · 2022-10-24T12:19:57Z

core/src/main/scala/org/apache/spark/SparkThrowableHelper.scala

@cloud-fan this follows the standard error msg json format

mridulm · 2022-10-24T17:12:12Z

+CC @shardulm94, @thejdeep

cloud-fan

LGTM. cc @gengliangwang @MaxGekk

gengliangwang · 2022-10-25T08:04:15Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLJsonProtocolSuite.scala

I find this \"messageParameters\":{\"message\":\"test\"} odd...
Shouldn't it just be an error message?

I just follow the style of error json format, so we can keep consistent with SparkThrowable. We'd expect all java throwables are wrapped by SparkThrowable so we will never go into here.

ulysses-you · 2022-11-03T10:24:01Z

@gengliangwang @cloud-fan do you have any concerns about this pr ?

cloud-fan · 2022-11-03T13:02:40Z

thanks, merging to master!

cloud-fan · 2022-11-21T16:48:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala

+case class SparkListenerSQLExecutionEnd(
+    executionId: Long,
+    time: Long,
+    errorMessage: Option[String] = None)


Thinking about this more, I think we should use Some("") to indicate no error, as None may mean event logs produced by old version of Spark that don't have this error message field. @ulysses-you

I made a followup: #38747

HyukjinKwon · 2022-11-22T06:18:43Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala

+case class SparkListenerSQLExecutionEnd(
+    executionId: Long,
+    time: Long,
+    errorMessage: Option[String] = None)


Doesn't this break binary compatibility? I think it should have overridden constructor.

This is not a public class. It generates JSON string where backward compatibility matters, and this PR added tests for it.

### What changes were proposed in this pull request? This is a followup of #38302 . For events generated by old versions of Spark, which do not have the new `errorMessage` field, we should use the old way to detect query execution status (failed or not). This PR also adds a UI test for the expected behavior. ### Why are the changes needed? backward compatibility ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new tests Closes #38747 from cloud-fan/ui. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…L status in UI ### What changes were proposed in this pull request? Use event `SparkListenerSQLExecutionEnd` to track if the SQL/DataFrame is completion instead of using job status. ### Why are the changes needed? The SQL may succeed with some failed jobs. For example, a inner join with one empty side and one large side, the plan would finish and the large side is still running. ### Does this PR introduce _any_ user-facing change? yes, correct the sql status in UI ### How was this patch tested? add test for backward compatibility and manually test ```sql CREATE TABLE t1 (c1 int) USING PARQUET; CREATE TABLE t2 USING PARQUET AS SELECT 1 AS c2; ``` ```bash ./bin/spark-sql -e "SELECT /*+ merge(tmp) */ * FROM t1 JOIN (SELECT c2, java_method('java.lang.Thread', 'sleep', 10000L) FROM t2) tmp ON c1 = c2;" ``` before: <img width="1712" alt="image" src="https://user-images.githubusercontent.com/12025282/196576790-7e4eeb29-024f-4ac3-bdec-f4e894448b57.png"> after: <img width="1709" alt="image" src="https://user-images.githubusercontent.com/12025282/196576674-15d80366-bd42-417b-80bf-eeec0b1ef046.png"> Closes apache#38302 from ulysses-you/sql-end. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a followup of apache#38302 . For events generated by old versions of Spark, which do not have the new `errorMessage` field, we should use the old way to detect query execution status (failed or not). This PR also adds a UI test for the expected behavior. ### Why are the changes needed? backward compatibility ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new tests Closes apache#38747 from cloud-fan/ui. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

allisonport-db · 2023-01-26T00:28:00Z

hi @ulysses-you any chance you can take a look at https://issues.apache.org/jira/browse/SPARK-41735?

github-actions bot added SQL WEB UI labels Oct 19, 2022

ulysses-you commented Oct 19, 2022

View reviewed changes

ulysses-you force-pushed the sql-end branch from c2b518c to f378e17 Compare October 24, 2022 12:18

ulysses-you commented Oct 24, 2022

View reviewed changes

github-actions bot added the CORE label Oct 24, 2022

cloud-fan approved these changes Oct 25, 2022

View reviewed changes

gengliangwang reviewed Oct 25, 2022

View reviewed changes

ulysses-you added 3 commits October 27, 2022 10:40

Use SparkListenerSQLExecutionEnd to track final SQL status in UI

0cdfd12

fix

1ba0324

json format

02a5b04

ulysses-you force-pushed the sql-end branch from f378e17 to 02a5b04 Compare October 27, 2022 02:41

cloud-fan closed this in 0745dae Nov 3, 2022

ulysses-you deleted the sql-end branch November 4, 2022 01:20

cloud-fan reviewed Nov 21, 2022

View reviewed changes

cloud-fan mentioned this pull request Nov 21, 2022

[SPARK-40834][SQL][FOLLOWUP] Take care of legacy query end events #38747

Closed

HyukjinKwon reviewed Nov 22, 2022

View reviewed changes

CxwXiaoWei mentioned this pull request Nov 23, 2022

[SPARK-40834][SQL][FOLLOWUP] Take care of legacy query end events CxwXiaoWei/spark#1

Merged

ulysses-you mentioned this pull request Jun 9, 2023

[SPARK-37043][SQL] Cancel all running job when main query is going to final plan #41536

Closed

[SPARK-40834][SQL] Use SparkListenerSQLExecutionEnd to track final SQL status in UI #38302

[SPARK-40834][SQL] Use SparkListenerSQLExecutionEnd to track final SQL status in UI #38302

Uh oh!

Conversation

ulysses-you commented Oct 19, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulysses-you commented Oct 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mridulm commented Oct 24, 2022

Uh oh!

cloud-fan left a comment

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!

ulysses-you commented Nov 3, 2022

Uh oh!

cloud-fan commented Nov 3, 2022

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!

allisonport-db commented Jan 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants