[SPARK-45798][CONNECT] Assert server-side session ID by grundprinzip · Pull Request #43664 · apache/spark

grundprinzip · 2023-11-05T11:09:19Z

What changes were proposed in this pull request?

Without this patch, when the server would restart because of an abnormal condition, the client would not realize that this be the case. For example, when a driver OOM occurs and the driver is restarted, the client would not realize that the server is restarted and a new session is assigned.

This patch fixes this behavior and asserts that the server side session ID does not change during the connection. If it does change it throws an exception like this:

>>> spark.range(10).collect()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/dataframe.py", line 1710, in collect
    table, schema = self._to_table()
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/dataframe.py", line 1722, in _to_table
    table, schema = self._session.client.to_table(query, self._plan.observations)
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 839, in to_table
    table, schema, _, _, _ = self._execute_and_fetch(req, observations)
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 1295, in _execute_and_fetch
    for response in self._execute_and_fetch_as_iterator(req, observations):
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 1273, in _execute_and_fetch_as_iterator
    self._handle_error(error)
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 1521, in _handle_error
    raise error
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 1266, in _execute_and_fetch_as_iterator
    yield from handle_response(b)
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 1193, in handle_response
    self._verify_response_integrity(b)
  File "/Users/martin.grund/Development/spark/python/pyspark/sql/connect/client/core.py", line 1622, in _verify_response_integrity
    raise SparkConnectException(
pyspark.errors.exceptions.connect.SparkConnectException: Received incorrect server side session identifier for request. Please restart Spark Session. (9493c83d-cfa4-488f-9522-838ef3df90bf != c5302e8f-170d-477e-908d-299927b68fd8)

Why are the changes needed?

Stability

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests cover the basic invariant.
Added new tests.

Was this patch authored or co-authored using generative AI tooling?

No

connector/connect/common/src/main/protobuf/spark/connect/base.proto

...mmon/src/main/scala/org/apache/spark/sql/connect/client/CustomSparkConnectBlockingStub.scala

...main/scala/org/apache/spark/sql/connect/client/ExecutePlanResponseReattachableIterator.scala

connector/connect/common/src/main/protobuf/spark/connect/base.proto

...ctor/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala

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

python/pyspark/sql/connect/client/core.py

...onnect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectStubState.scala

...ver/src/test/scala/org/apache/spark/sql/connect/service/SparkConnectSessionHolderSuite.scala

...onnect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectStubState.scala

juliuszsompolski

LGTM
Would be good if someone knowing python looked at it (@HyukjinKwon ?) but it looks good to me.

juliuszsompolski · 2023-11-08T18:05:51Z

https://github.com/grundprinzip/spark/actions/runs/6796293583/job/18489636721 looks like it flaked out after already running all connect tests successfuly.

HyukjinKwon · 2023-11-10T01:32:51Z

Merged to master.

…Info ### What changes were proposed in this pull request? Small followup to #43664 - add serverSessionId to SessionHolderInfo. ### Why are the changes needed? SessionHolderInfo should contain this kind of information about the session. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? NA ### Was this patch authored or co-authored using generative AI tooling? No. Closes #44334 from juliuszsompolski/SPARK-45798-fup. Authored-by: Juliusz Sompolski <julek@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

[SPARK-45798] Assert server-side session ID

0c0deeb

github-actions bot added SQL PYTHON CONNECT labels Nov 5, 2023

grundprinzip added 2 commits November 5, 2023 12:36

Merge remote-tracking branch 'origin/master' into HEAD

3324b43

next

96a91ea

dongjoon-hyun changed the title ~~[SPARK-45798] Assert server-side session ID~~ [SPARK-45798][CONNECT] Assert server-side session ID Nov 5, 2023

grundprinzip added 2 commits November 5, 2023 22:26

fixing artifact stuff

ddbc145

fix NPE

ac428f5

juliuszsompolski reviewed Nov 6, 2023

View reviewed changes

...ctor/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala Show resolved Hide resolved

juliuszsompolski reviewed Nov 6, 2023

View reviewed changes

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

grundprinzip added 4 commits November 6, 2023 13:10

fix ut

cea5057

Merge remote-tracking branch 'origin/master' into SPARK-45798

4cdeb01

fix observer

2f56aef

fix

bc068e7

allisonwang-db reviewed Nov 7, 2023

View reviewed changes

python/pyspark/sql/connect/client/core.py Outdated Show resolved Hide resolved

python/pyspark/sql/connect/client/core.py Outdated Show resolved Hide resolved

grundprinzip added 2 commits November 7, 2023 08:59

fix

f50f4ea

fix

01d5b1f

juliuszsompolski reviewed Nov 7, 2023

View reviewed changes

...onnect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectStubState.scala Outdated Show resolved Hide resolved

juliuszsompolski reviewed Nov 7, 2023

View reviewed changes

...onnect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectStubState.scala Outdated Show resolved Hide resolved

juliuszsompolski reviewed Nov 7, 2023

View reviewed changes

...onnect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectStubState.scala Outdated Show resolved Hide resolved

grundprinzip added 2 commits November 7, 2023 14:56

refactoring

e59389f

refactoring

d1df182

juliuszsompolski reviewed Nov 7, 2023

View reviewed changes

...ver/src/test/scala/org/apache/spark/sql/connect/service/SparkConnectSessionHolderSuite.scala Outdated Show resolved Hide resolved

...onnect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectStubState.scala Outdated Show resolved Hide resolved

grundprinzip added 3 commits November 7, 2023 18:36

Merge remote-tracking branch 'origin/master' into SPARK-45798

1bb0727

Merge remote-tracking branch 'origin/master' into SPARK-45798

c0c6212

fix

7c84027

juliuszsompolski approved these changes Nov 7, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into SPARK-45798

610ef1e

fix

c33526c

juliuszsompolski approved these changes Nov 8, 2023

View reviewed changes

HyukjinKwon approved these changes Nov 10, 2023

View reviewed changes

HyukjinKwon closed this in d9c5f9d Nov 10, 2023

juliuszsompolski mentioned this pull request Nov 23, 2023

[SPARK-46061][PYTHON][TESTS] Add the test party for reattach test case #43965

Closed

juliuszsompolski mentioned this pull request Dec 13, 2023

[SPARK-45798][CONNECT] Followup: add serverSessionId to SessionHolderInfo #44334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-45798][CONNECT] Assert server-side session ID#43664

[SPARK-45798][CONNECT] Assert server-side session ID#43664
grundprinzip wants to merge 18 commits intoapache:masterfrom
grundprinzip:SPARK-45798

grundprinzip commented Nov 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliuszsompolski left a comment

Uh oh!

juliuszsompolski commented Nov 8, 2023

Uh oh!

HyukjinKwon commented Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

grundprinzip commented Nov 5, 2023

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?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

juliuszsompolski commented Nov 8, 2023

Uh oh!

HyukjinKwon commented Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments