[SPARK-48810][CONNECT] Session stop() API should be idempotent and not fail if the session is already closed by the server. #47215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grundprinzip
reviewed
Jul 4, 2024
Comment on lines
+851
to
+857
| try: | ||
| PySparkSession._activeSession.stop() | ||
| except Exception as e: | ||
| warnings.warn( | ||
| "session.stop(): Local Spark Connect Server could not be stopped. " | ||
| f"Error: ${e}" | ||
| ) |
There was a problem hiding this comment.
I'm wondering if this one actually needs to be wrapped because. If I'm reading this correctly, the PySparkSession._activeSession is actually a Java Session.
There was a problem hiding this comment.
It is the underlying Java session.
Unclear. Felt safer such as in the case that the JVM is killed out of band. I don't know all the failure modes in this code path.
grundprinzip
reviewed
Jul 4, 2024
nija-at
changed the title
grundprinzip
approved these changes
Jul 4, 2024
cloud-fan
approved these changes
Jul 8, 2024
|
thanks, merging to master! |
|
@nija-at please open a 3.5 backport PR if needed, thanks! |
|
wow @nija-at . please add this to 3.5 as well. |
ericm-db
pushed a commit
to ericm-db/spark
that referenced
this pull request
Jul 10, 2024
…t fail if the session is already closed by the server ### What changes were proposed in this pull request? Improve the error handling of the `stop()` API in the `SparkSesion` class to not throw if there is any error related to releasing a session or closing the underlying GRPC channel. Both are best effort. In the case of Pyspark, do not fail if the local Spark Connect service cannot be stopped. ### Why are the changes needed? In some cases, the Spark Connect Service will terminate the session, usually because the underlying cluster or driver has restarted. In the cases, calling stop() throws an error which is unactionable. However, stop() still needs to be called in order to reset the active session. Further, the stop() API should be idempotent. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Attached unit tests. Confirmed that removing the code changes results in the tests failing (as expected). ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47215 from nija-at/session-stop. Authored-by: Niranjan Jayakar <nija@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
jingz-db
pushed a commit
to jingz-db/spark
that referenced
this pull request
Jul 22, 2024
…t fail if the session is already closed by the server ### What changes were proposed in this pull request? Improve the error handling of the `stop()` API in the `SparkSesion` class to not throw if there is any error related to releasing a session or closing the underlying GRPC channel. Both are best effort. In the case of Pyspark, do not fail if the local Spark Connect service cannot be stopped. ### Why are the changes needed? In some cases, the Spark Connect Service will terminate the session, usually because the underlying cluster or driver has restarted. In the cases, calling stop() throws an error which is unactionable. However, stop() still needs to be called in order to reset the active session. Further, the stop() API should be idempotent. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Attached unit tests. Confirmed that removing the code changes results in the tests failing (as expected). ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47215 from nija-at/session-stop. Authored-by: Niranjan Jayakar <nija@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
attilapiros
pushed a commit
to attilapiros/spark
that referenced
this pull request
Oct 4, 2024
…t fail if the session is already closed by the server ### What changes were proposed in this pull request? Improve the error handling of the `stop()` API in the `SparkSesion` class to not throw if there is any error related to releasing a session or closing the underlying GRPC channel. Both are best effort. In the case of Pyspark, do not fail if the local Spark Connect service cannot be stopped. ### Why are the changes needed? In some cases, the Spark Connect Service will terminate the session, usually because the underlying cluster or driver has restarted. In the cases, calling stop() throws an error which is unactionable. However, stop() still needs to be called in order to reset the active session. Further, the stop() API should be idempotent. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Attached unit tests. Confirmed that removing the code changes results in the tests failing (as expected). ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47215 from nija-at/session-stop. Authored-by: Niranjan Jayakar <nija@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Improve the error handling of the
stop()API in theSparkSesionclass to not throw if there is any error related to releasing a session or
closing the underlying GRPC channel. Both are best effort.
In the case of Pyspark, do not fail if the local Spark Connect service
cannot be stopped.
Why are the changes needed?
In some cases, the Spark Connect Service will terminate the session, usually
because the underlying cluster or driver has restarted.
In the cases, calling stop() throws an error which is unactionable. However,
stop() still needs to be called in order to reset the active session.
Further, the stop() API should be idempotent.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Attached unit tests.
Confirmed that removing the code changes results in the
tests failing (as expected).
Was this patch authored or co-authored using generative AI tooling?
No.