You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "haoyangeng-db (via GitHub)" <gi...@apache.org> on 2026/08/07 05:06:39 UTC
[PR] [SPARK-58629][CONNECT] Avoid empty fallback gRPC errors [spark]
haoyangeng-db opened a new pull request, #57837:
URL: https://github.com/apache/spark/pull/57837
### What changes were proposed in this pull request?
This PR makes Spark Connect's fallback gRPC error description non-empty.
When a throwable has a non-empty message, the message remains abbreviated to 2048 characters. Otherwise, the throwable's fully qualified class name is used.
A regression test covers a bare `InterruptedException`, which has no message.
### Why are the changes needed?
Fatal throwables such as `InterruptedException` reach the generic fallback path. `Utils.abbreviate(null, 2048)` returns `null`, causing Spark Connect to emit an `UNKNOWN` status without a useful description.
Clients consequently receive an opaque error. This change preserves the status code while providing the throwable class as useful diagnostic context.
### Does this PR introduce _any_ user-facing change?
Yes. For an already-failing request whose fatal throwable has no message, the client now receives the throwable class name instead of an empty error description. Successful requests and other error-conversion paths are unchanged.
### How was this patch tested?
Added a regression test to `FetchErrorDetailsHandlerSuite` verifying that a bare `InterruptedException` produces an `UNKNOWN` status with `java.lang.InterruptedException` as its description.
The focused suite could not run locally because the required SBT dependencies were unavailable in the development environment. Static diff checks passed; the regression will also run in CI.
### Was this patch authored or co-authored using generative AI tooling?
Co-authord with Codex.
--
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
Re: [PR] [SPARK-58629][CONNECT] Avoid empty fallback gRPC errors [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #57837:
URL: https://github.com/apache/spark/pull/57837#issuecomment-5346297407
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/f167d371629c0ea5c8ff01a783215d1dc7d96fec
- merged into branch-4.x https://github.com/apache/spark/commit/c3b200ed70b904b53d64737812d37e2f9b6f18c8
*Posted by `merge_spark_pr.py`*
--
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
Re: [PR] [SPARK-58629][CONNECT] Avoid empty fallback gRPC errors [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57837:
URL: https://github.com/apache/spark/pull/57837#issuecomment-5215965079
Thank you @haoyangeng-db!
--
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
Re: [PR] [SPARK-58629][CONNECT] Avoid empty fallback gRPC errors [spark]
Posted by "haoyangeng-db (via GitHub)" <gi...@apache.org>.
haoyangeng-db commented on PR #57837:
URL: https://github.com/apache/spark/pull/57837#issuecomment-5259397776
> +1, LGTM. The fix is correct and minimal: `InterruptedException` is fatal per `NonFatal` and is not a `SparkThrowable`, so it reaches the fallback path where `Utils.abbreviate(null, 2048)` previously produced a description-less `UNKNOWN` status. Keeping the status code while falling back to the throwable's class name is the right call, and the message-present behavior is preserved.
>
> A few non-blocking comments:
>
> * The same gap remains on the non-fatal path: `SparkConnectService.extractErrorMessage` converts a null message to an empty string, so a message-less non-fatal exception (e.g. a bare `new RuntimeException()`) still yields an `INTERNAL` status with an empty message. Might be worth the same class-name fallback in a follow-up.
> * The new test verifies `ErrorUtils.handleError` rather than the `FetchErrorDetails` handler, so `SparkConnectServiceSuite` (or a dedicated `ErrorUtilsSuite`) would be a more natural home; it is also inserted between the suite's field declarations and helper defs.
> * Nit: an `assert(emittedError != null)` (or `isInstanceOf` check) before the cast would give a clearer failure message if `onError` were never invoked.
> * Alternative worth noting: `Utils.abbreviate(e.toString, 2048)` gives exactly "class: message" or the class name per the `Throwable.toString` contract, though it would change the existing message-present output, so keeping the current approach is reasonable too.
Thanks @dongjoon-hyun ! Moved the regression test into a dedicated ErrorUtilsSuite, and replaced the nullable capture/cast with intercept[StatusRuntimeException]. Kept the current getMessage/class-name fallback so errors with message retain their existing description format.
I can look into the non-fatal INTERNAL path in a separate follow-up.
Does this sound good to you?
--
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
Re: [PR] [SPARK-58629][CONNECT] Avoid empty fallback gRPC errors [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #57837: [SPARK-58629][CONNECT] Avoid empty fallback gRPC errors
URL: https://github.com/apache/spark/pull/57837
--
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