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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
