You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "allisonwang-db (via GitHub)" <gi...@apache.org> on 2023/03/23 10:37:37 UTC
[GitHub] [spark] allisonwang-db opened a new pull request, #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
allisonwang-db opened a new pull request, #40536:
URL: https://github.com/apache/spark/pull/40536
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR improves error messages when users attempt to invoke session operations on a stopped Spark session.
### Why are the changes needed?
To make the error messages more user-friendly.
For example:
```python
spark.stop()
spark.sql("select 1")
```
Before this PR, this code will throw two exceptions:
```
ValueError: Cannot invoke RPC: Channel closed!
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
...
return e.code() == grpc.StatusCode.UNAVAILABLE
AttributeError: 'ValueError' object has no attribute 'code'
```
After this PR, it will show this exception:
```
pyspark.errors.exceptions.base.PySparkValueError: [REMOTE_SESSION_STOPPED]
No active Spark session found. Please create a new Spark session before running the code.
```
### Does this PR introduce _any_ user-facing change?
Yes. This PR modifies the error messages.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
New unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1150045613
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,32 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION", message_parameters=dict()
+ )
Review Comment:
I think Python exception chaining would work here so it'd be fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1150013541
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,32 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION", message_parameters=dict()
+ )
Review Comment:
I am thinking this will swallow other ValueError? You probably need to re-throw non-closed channel ValueError?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] allisonwang-db commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1147266298
##########
python/pyspark/sql/connect/client.py:
##########
@@ -513,8 +513,11 @@ class SparkConnectClient(object):
"""
@classmethod
- def retry_exception(cls, e: grpc.RpcError) -> bool:
- return e.code() == grpc.StatusCode.UNAVAILABLE
+ def retry_exception(cls, e: Exception) -> bool:
+ if isinstance(e, grpc.RpcError):
+ return e.code() == grpc.StatusCode.UNAVAILABLE
+ else:
+ return False
Review Comment:
> why don't we just throw an exception here?
The AttemptManager will bubble up the exception when retry_exception is false.
> I am sure there are more places than just SparkSession that does the connection to the server side, e.g.,
DataFrame.collect()
Good point. I will check other places too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1148919308
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,36 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION",
+ message_parameters=dict()
+ )
+ else:
+ raise error
+ else:
+ raise error
Review Comment:
```suggestion
raise error
```
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,36 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION",
+ message_parameters=dict()
+ )
+ else:
+ raise error
+ else:
+ raise error
Review Comment:
but no biggie
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] allisonwang-db commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1150302155
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,32 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION", message_parameters=dict()
+ )
Review Comment:
Yea it can throw other `ValueError`s. And here is what it looks like in console:
```
pyspark.errors.exceptions.connect.SparkConnectException: [NO_ACTIVE_SESSION] No active Spark session found. Please create a new Spark session before running the code.
```
To verify this, I manually changed the code to throw a ValueError. Is there a way to trigger other GRPC Cannot invoke RPC errors?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1147056718
##########
python/pyspark/sql/connect/client.py:
##########
@@ -513,8 +513,11 @@ class SparkConnectClient(object):
"""
@classmethod
- def retry_exception(cls, e: grpc.RpcError) -> bool:
- return e.code() == grpc.StatusCode.UNAVAILABLE
+ def retry_exception(cls, e: Exception) -> bool:
+ if isinstance(e, grpc.RpcError):
+ return e.code() == grpc.StatusCode.UNAVAILABLE
+ else:
+ return False
Review Comment:
Hm, why don't we just throw an exception here?
I am sure there are more places than just `SparkSession` that does the connection to the server side, e.g., `DataFrame.collect()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1151540599
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,32 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION", message_parameters=dict()
+ )
Review Comment:
I think that's good enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40536:
URL: https://github.com/apache/spark/pull/40536#discussion_r1150045902
##########
python/pyspark/sql/connect/client.py:
##########
@@ -997,10 +1000,32 @@ def config(self, operation: pb2.ConfigRequest.Operation) -> ConfigResult:
)
return ConfigResult.fromProto(resp)
raise SparkConnectException("Invalid state during retry exception handling.")
- except grpc.RpcError as rpc_error:
- self._handle_error(rpc_error)
+ except Exception as error:
+ self._handle_error(error)
+
+ def _handle_error(self, error: Exception) -> NoReturn:
+ """
+ Handle errors that occur during RPC calls.
+
+ Parameters
+ ----------
+ error : Exception
+ An exception thrown during RPC calls.
+
+ Returns
+ -------
+ Throws the appropriate internal Python exception.
+ """
+ if isinstance(error, grpc.RpcError):
+ self._handle_rpc_error(cast(grpc.RpcError, error))
+ elif isinstance(error, ValueError):
+ if "Cannot invoke RPC" in str(error) and "closed" in str(error):
+ raise SparkConnectException(
+ error_class="NO_ACTIVE_SESSION", message_parameters=dict()
+ )
Review Comment:
Would be great to double check how it looks like in the console.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
URL: https://github.com/apache/spark/pull/40536
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #40536: [SPARK-42895][CONNECT] Improve error messages for stopped Spark sessions
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40536:
URL: https://github.com/apache/spark/pull/40536#issuecomment-1488120272
Merged to master and branch-3.4.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org