[SPARK-43923][CONNECT] Post listenerBus events during ExecutePlanRequest #41443

jdesjean · 2023-06-03T00:44:27Z

What changes were proposed in this pull request?

Add new SparkListenerEvent during Spark Connect ExecutePlanRequest:
SparkListenerConnectOperationStarted
SparkListenerConnectOperationParsed
SparkListenerConnectOperationCanceled,
SparkListenerConnectOperationFailed
SparkListenerConnectOperationFinished
SparkListenerConnectOperationClosed
SparkListenerConnectSessionClosed .

Why are the changes needed?

HiveThriftServer2EventManager currently posts events to the listener bus to allow external listeners to track query execution. Mirror these events in Spark Connect.
Created new events instead of reusing the thrift events to allow them to evolve separately.

Does this PR introduce any user-facing change?

How was this patch tested?

Manual + Unit + E2E

connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/Events.scala

...connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala

...r/connect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecutePlanHolder.scala

connector/connect/server/src/test/scala/org/apache/spark/sql/connect/service/EventsSuite.scala

...nect/server/src/test/scala/org/apache/spark/sql/connect/service/ExecutePlanHolderSuite.scala

beliefer · 2023-06-12T04:47:00Z

Personally, I think Connect is front of Spark Runtime. ExecutePlanRequest should be transparent to Spark Runtime.

hvanhovell · 2023-06-14T20:06:14Z

@juliuszsompolski FYI

hvanhovell · 2023-06-14T20:06:55Z

@beliefer I am not sure what you are saying here? Can you elaborate?

connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/Events.scala

...r/connect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecutePlanHolder.scala

...t/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectStreamHandler.scala

juliuszsompolski · 2023-06-14T20:45:36Z

Questions comparing to Thriftserver state transitions:

Do we want to distinguish STARTED (when the request starts being processed, before PARSED) and RUNNING (for plan executions that would be when submitJob is called?)
Thriftserver distinguishes FINISHED (when execution finished, and results are still being send out) vs. CLOSED (when the results iterator is fully closed). The difference is that Thriftserver first collects all results, and only then starts sending them out. Spark Connect starts sending results as soon as some are available. We could however consider a "FINISHED" state, when all results are available, but yet being send out. This could be done in https://github.com/apache/spark/pull/41443/files#diff-9d1e76581c17b7c18f067ff764493494eb40eeb5787e8c3cb6e5422218802372R214 when the resultHandler sees that it received all numPartitions.

beliefer · 2023-06-15T05:43:49Z

@beliefer I am not sure what you are saying here? Can you elaborate?

Spark Driver is an independent and stable system. The Spark driver uses SparkListenerEvents independently. Spark has many application, like SQL, MLlib, Python and so on. All the applications are on top of Spark's architecture. We should keep the independence of the application(e.g. Connect) and spark.

On the other hand, SparkListenerEvent is used by Spark schedule but the state of Connect operation(e.g. cancel).

...ct/server/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectServiceSuite.scala

juliuszsompolski

LGTM

HyukjinKwon · 2023-07-18T00:04:17Z

Merged to master, and branch-3.5.

### What changes were proposed in this pull request? Add new SparkListenerEvent during Spark Connect ExecutePlanRequest: SparkListenerConnectOperationStarted SparkListenerConnectOperationParsed SparkListenerConnectOperationCanceled, SparkListenerConnectOperationFailed SparkListenerConnectOperationFinished SparkListenerConnectOperationClosed SparkListenerConnectSessionClosed . ### Why are the changes needed? HiveThriftServer2EventManager currently posts events to the listener bus to allow external listeners to track query execution. Mirror these events in Spark Connect. Created new events instead of reusing the thrift events to allow them to evolve separately. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? Manual + Unit + E2E Closes #41443 from jdesjean/SPARK-43923. Authored-by: jdesjean <jf.gauthier@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit b44e605) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

HyukjinKwon · 2023-07-18T08:23:35Z

Hm, I think this makes this test flaky: https://github.com/apache/spark/actions/runs/5581822850/jobs/10201652403 and seems pretty often. Would you mind taking a look @jdesjean ?

HyukjinKwon · 2023-07-18T08:27:40Z

Let me skip the test for now - it blocks other PRs, and I don't want to revert this. I made a JIRA at https://issues.apache.org/jira/browse/SPARK-44474 to reanble the test, and made it as a blocker of Spark 3.5.0.

jdesjean · 2023-07-18T17:44:53Z

@HyukjinKwon, I'm reenabling the test with a fix

…ctServiceSuite ### What changes were proposed in this pull request? Finished is emitted in SparkConnectPlanExecution after the arrow conversion job is completed. However, since we don't await the completion of the job, it's possible for SparkConnectPlanExecution to complete before sending Finished. Closed is emitted in SparkConnectExecutePlanHandler in a separate thread. Add await in order to guarantee the order of events between Finished & Closed. ### Why are the changes needed? `Test observe response` at SparkConnectServiceSuite was disabled as flaky after [introduction of events](#41443). Failure surfaced race condition in emitting the Finished & Closed events for Connect request of type plan. The correct order of events is Finished < Closed. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit Closes #42063 from jdesjean/SPARK-44474. Authored-by: jdesjean <jf.gauthier@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…ctServiceSuite ### What changes were proposed in this pull request? Finished is emitted in SparkConnectPlanExecution after the arrow conversion job is completed. However, since we don't await the completion of the job, it's possible for SparkConnectPlanExecution to complete before sending Finished. Closed is emitted in SparkConnectExecutePlanHandler in a separate thread. Add await in order to guarantee the order of events between Finished & Closed. ### Why are the changes needed? `Test observe response` at SparkConnectServiceSuite was disabled as flaky after [introduction of events](#41443). Failure surfaced race condition in emitting the Finished & Closed events for Connect request of type plan. The correct order of events is Finished < Closed. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit Closes #42063 from jdesjean/SPARK-44474. Authored-by: jdesjean <jf.gauthier@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit cf99e6c) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

## What changes were proposed in this pull request? Add a new Spark UI page to display session and execution information for Spark Connect. This builds of the work in SPARK-43923 (#41443) that adds the relevant SparkListenerEvents and mirrors the ThriftServerPage in the Spark UI for JDBC/ODBC. <img width="1709" alt="Screenshot 2023-07-27 at 11 29 22 PM" src="https://github.com/apache/spark/assets/65624911/934b7c69-3b44-460b-8fbb-36a9eb3f0798"> <img width="1716" alt="Screenshot 2023-07-27 at 11 29 15 PM" src="https://github.com/apache/spark/assets/65624911/33dbe6ab-44bf-49a5-ad4c-5ba4a476a1f0"> ### Why are the changes needed? This gives users a way to access session and execution information for Spark Connect via the UI and provides the frontend interface for the related SparkListenerEvents. ### Does this PR introduce _any_ user-facing change? Yes, it will add a new tab/page in the Spark UI ### How was this patch tested? Unit tests Closes #41964 from jasonli-db/spark-connect-ui. Authored-by: Jason Li <jason.li@databricks.com> Signed-off-by: Gengliang Wang <gengliang@apache.org>

## What changes were proposed in this pull request? Add a new Spark UI page to display session and execution information for Spark Connect. This builds of the work in SPARK-43923 (apache#41443) that adds the relevant SparkListenerEvents and mirrors the ThriftServerPage in the Spark UI for JDBC/ODBC. <img width="1709" alt="Screenshot 2023-07-27 at 11 29 22 PM" src="https://github.com/apache/spark/assets/65624911/934b7c69-3b44-460b-8fbb-36a9eb3f0798"> <img width="1716" alt="Screenshot 2023-07-27 at 11 29 15 PM" src="https://github.com/apache/spark/assets/65624911/33dbe6ab-44bf-49a5-ad4c-5ba4a476a1f0"> ### Why are the changes needed? This gives users a way to access session and execution information for Spark Connect via the UI and provides the frontend interface for the related SparkListenerEvents. ### Does this PR introduce _any_ user-facing change? Yes, it will add a new tab/page in the Spark UI ### How was this patch tested? Unit tests Closes apache#41964 from jasonli-db/spark-connect-ui. Authored-by: Jason Li <jason.li@databricks.com> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit f8786f0)

## What changes were proposed in this pull request? Add a new Spark UI page to display session and execution information for Spark Connect. This builds of the work in SPARK-43923 (#41443) that adds the relevant SparkListenerEvents and mirrors the ThriftServerPage in the Spark UI for JDBC/ODBC. <img width="1709" alt="Screenshot 2023-07-27 at 11 29 22 PM" src="https://github.com/apache/spark/assets/65624911/934b7c69-3b44-460b-8fbb-36a9eb3f0798"> <img width="1716" alt="Screenshot 2023-07-27 at 11 29 15 PM" src="https://github.com/apache/spark/assets/65624911/33dbe6ab-44bf-49a5-ad4c-5ba4a476a1f0"> ### Why are the changes needed? This gives users a way to access session and execution information for Spark Connect via the UI and provides the frontend interface for the related SparkListenerEvents. ### Does this PR introduce _any_ user-facing change? Yes, it will add a new tab/page in the Spark UI ### How was this patch tested? Unit tests Closes #41964 from jasonli-db/spark-connect-ui. Authored-by: Jason Li <jason.lidatabricks.com> Signed-off-by: Gengliang Wang <gengliangapache.org> (cherry picked from commit f8786f0) Closes #42224 from juliuszsompolski/SPARK-44394-3.5. Authored-by: Jason Li <jason.li@databricks.com> Signed-off-by: Gengliang Wang <gengliang@apache.org>

github-actions bot added CONNECT SQL labels Jun 3, 2023

jdesjean force-pushed the SPARK-43923 branch 2 times, most recently from ec58f21 to e1964e1 Compare June 3, 2023 14:51

nija-at reviewed Jun 5, 2023

View reviewed changes

jdesjean force-pushed the SPARK-43923 branch 6 times, most recently from 9c6dcf3 to 996dc3a Compare June 9, 2023 04:02

jdesjean requested a review from nija-at June 9, 2023 04:17

jdesjean force-pushed the SPARK-43923 branch 2 times, most recently from 91df042 to ee8e380 Compare June 9, 2023 17:00

juliuszsompolski reviewed Jun 14, 2023

View reviewed changes

...t/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectStreamHandler.scala Outdated Show resolved Hide resolved

jdesjean mentioned this pull request Jun 15, 2023

[SPARK-43952][CORE][CONNECT][SQL] Add SparkContext APIs for query cancellation by tag #41440

Closed

jdesjean force-pushed the SPARK-43923 branch 8 times, most recently from bfe2b73 to efa548a Compare June 16, 2023 20:58

jdesjean added 6 commits July 17, 2023 08:01

executeEventsManager -> eventsManager

dffd26b

Comment + logger

a671582

Streaming commands unit tests

ee2e8cc

Thread syntax

bccdee9

Moved canceled to handleError

c1862e1

Sleep -> semaphore

980dcb5

jdesjean force-pushed the SPARK-43923 branch from 7a9cc6d to 980dcb5 Compare July 17, 2023 15:25

juliuszsompolski reviewed Jul 17, 2023

View reviewed changes

...ct/server/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectServiceSuite.scala Outdated Show resolved Hide resolved

Move status assertions to eventsManager

7422e7b

jdesjean force-pushed the SPARK-43923 branch from c0f055c to 7422e7b Compare July 17, 2023 19:08

juliuszsompolski approved these changes Jul 17, 2023

View reviewed changes

Fix SparkConnectProtoSuite, assert session status

14266f7

jdesjean force-pushed the SPARK-43923 branch from ba6f979 to 14266f7 Compare July 17, 2023 21:42

HyukjinKwon approved these changes Jul 17, 2023

View reviewed changes

HyukjinKwon closed this in b44e605 Jul 18, 2023

jdesjean mentioned this pull request Jul 18, 2023

[SPARK-44474][CONNECT] Reenable "Test observe response" at SparkConnectServiceSuite #42063

Closed

juliuszsompolski mentioned this pull request Jul 29, 2023

[SPARK-44394][CONNECT][WEBUI] Add a Spark UI page for Spark Connect #42224

Closed

[SPARK-43923][CONNECT] Post listenerBus events during ExecutePlanRequest #41443

[SPARK-43923][CONNECT] Post listenerBus events during ExecutePlanRequest #41443

Uh oh!

Conversation

jdesjean commented Jun 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beliefer commented Jun 12, 2023

Uh oh!

hvanhovell commented Jun 14, 2023

Uh oh!

hvanhovell commented Jun 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliuszsompolski commented Jun 14, 2023

Uh oh!

beliefer commented Jun 15, 2023

Uh oh!

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdesjean commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jdesjean commented Jun 3, 2023 •

edited

Loading

HyukjinKwon commented Jul 18, 2023 •

edited

Loading

HyukjinKwon commented Jul 18, 2023 •

edited

Loading

HyukjinKwon commented Jul 18, 2023 •

edited

Loading