pan3793 commented on code in PR #57009:
URL: https://github.com/apache/spark/pull/57009#discussion_r3674458143
##########
sql/connect/client/jdbc/src/main/scala/org/apache/spark/sql/connect/client/jdbc/util/JdbcErrorUtils.scala:
##########
@@ -53,4 +59,65 @@ private[jdbc] object JdbcErrorUtils {
case _ =>
throw new IllegalArgumentException(s"Invalid fetch direction:
$direction")
}
+
+ // SQLState class 08 is "connection exception"; HYT00 is the conventional
+ // (ODBC-derived) state for an elapsed timeout.
+ private val CONNECTION_DOES_NOT_EXIST = "08003"
+ private val CONNECTION_FAILURE = "08006"
+ private val TIMEOUT_EXPIRED = "HYT00"
+
+ private val SESSION_GONE_CONDITION_PREFIX = "INVALID_HANDLE.SESSION_"
+
+ /**
+ * Maps the unchecked exceptions raised by the Spark Connect client (a
+ * [[SparkThrowable]] once GrpcExceptionConverter has converted the gRPC
error) to
+ * the [[SQLException]] a JDBC method is required to throw:
+ *
+ * - `INVALID_HANDLE.SESSION_*` means the server-side session is gone (e.g.
it
+ * timed out); the connection is unusable and retrying on it is
pointless, so it
+ * maps to a [[SQLNonTransientConnectionException]] with SQLState 08003
+ * ("connection does not exist"). The operation-level subconditions
+ * (`OPERATION_*`, `FORMAT`) concern a single operation on a healthy
session
+ * and are deliberately not treated as connection errors.
+ * - a gRPC UNAVAILABLE (e.g. a server restart or a network blip) maps to a
+ * [[SQLTransientConnectionException]] with SQLState 08006 ("connection
+ * failure"), since a fresh connection can succeed.
+ * - a gRPC DEADLINE_EXCEEDED means the RPC deadline elapsed, which a slow
query
+ * fires on a perfectly healthy connection, so it maps to a
+ * [[SQLTimeoutException]] rather than a connection error.
+ * - any other error keeps the server-provided SQLState when one is
available.
+ *
+ * The gRPC status is read from the [[StatusRuntimeException]] that
+ * GrpcExceptionConverter preserves in the cause chain, never from message
text,
+ * so a server-side error merely quoting a gRPC exception cannot be mistaken
for
+ * a transport failure. SQLState class 08 is how connection pools and BI
tools
+ * detect a dead connection and reconnect.
+ */
Review Comment:
this looks reasonable to me, but does it mean that the current
`error-conditions.json` defines those errors in the wrong sqlState ?
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -170,23 +170,22 @@ private[client] class GrpcExceptionConverter(
}
// If no ErrorInfo is found, create a SparkException based on the
StatusRuntimeException.
- val (message, cause) = if (ex.getStatus.getCode ==
Status.Code.DEADLINE_EXCEEDED) {
- val msg = s"${ex.toString}: RPC deadline exceeded. Deadlines can be
configured via " +
+ val message = if (ex.getStatus.getCode == Status.Code.DEADLINE_EXCEEDED) {
+ s"${ex.toString}: RPC deadline exceeded. Deadlines can be configured via
" +
"SparkConnectClient.Builder.rpcDeadlines(). To disable all deadlines:
" +
"SparkConnectClient.builder().rpcDeadlines(RpcDeadlines.disabled).build()"
- // For DEADLINE_EXCEEDED, we pass `ex` itself as the cause rather than
`ex.getCause`.
- // StatusRuntimeException.getCause() returns status.getCause(), which is
always null for
- // client-side deadline fires (gRPC constructs the status without a
wrapped cause). Using
- // ex.getCause would produce a SparkException with cause = null, losing
the gRPC status
- // code and description from the exception chain. Passing ex preserves
full context and
- // allows callers to programmatically inspect the status code via
getCause().getStatus().
- (msg, ex)
} else {
- (ex.toString, ex.getCause)
+ ex.toString
}
+ // Pass `ex` itself as the cause rather than `ex.getCause`.
StatusRuntimeException.getCause()
+ // returns status.getCause(), which is often null (e.g. always for
client-side deadline
+ // fires, since gRPC constructs the status without a wrapped cause). Using
ex.getCause
+ // would produce a SparkException with cause = null, losing the gRPC
status code and
+ // description from the exception chain. Passing ex preserves full context
and allows
+ // callers to programmatically inspect the status code via
getCause().getStatus().
new SparkException(
message = message,
- cause = cause,
+ cause = ex,
Review Comment:
the change here looks fine to me:
1. Only the fallback path (no ErrorInfo) is changed. Server errors with
structured ErrorInfo return early at line 158-170, never reaching the changed
code. The vast majority of Spark Connect server errors carry ErrorInfo.
2. Only the cause field changes — from null to the SRE. The message, error
class, and parameters are identical. This is strictly more information, not a
semantic change.
##########
sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala:
##########
@@ -963,9 +991,8 @@ class SparkConnectClientSuite extends ConnectFunSuite {
scala.concurrent.Await.result(resultPromise.future, FiniteDuration(15,
TimeUnit.SECONDS))
}
// scalastyle:on awaitresult
- // A keepalive-triggered UNAVAILABLE carries no wrapped cause (same as
DEADLINE_EXCEEDED,
- // see GrpcExceptionConverter.toThrowable), so the status
code/description is only in the
- // message.
+ // The status code/description of a keepalive-triggered UNAVAILABLE is
part of the
+ // message (see GrpcExceptionConverter.toThrowable).
Review Comment:
please update the identical comment in
SparkConnectServiceKeepAliveSuite.scala:102-104
--
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]
