cloud-fan commented on code in PR #57202:
URL: https://github.com/apache/spark/pull/57202#discussion_r3580402546
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -408,7 +416,19 @@ object SparkConnectService extends Logging {
case _ => NettyServerBuilder.forPort(port)
}
sb.maxInboundMessageSize(SparkEnv.get.conf.get(CONNECT_GRPC_MAX_INBOUND_MESSAGE_SIZE).toInt)
- .addService(sparkConnectService)
+ if (SparkEnv.get.conf.get(CONNECT_GRPC_KEEPALIVE_ENABLED)) {
+ val keepAliveTimeSeconds =
SparkEnv.get.conf.get(CONNECT_GRPC_KEEPALIVE_TIME)
+ // Detects a silently-dead client connection (e.g. a NAT gateway/load
balancer dropping
+ // an idle connection mapping without sending a TCP RST/FIN) via
gRPC/HTTP2 keepalive
+ // PINGs.
+ sb.keepAliveTime(keepAliveTimeSeconds, TimeUnit.SECONDS)
+ sb.keepAliveTimeout(
+ SparkEnv.get.conf.get(CONNECT_GRPC_KEEPALIVE_TIMEOUT),
+ TimeUnit.SECONDS)
+ sb.permitKeepAliveTime(GRPC_KEEPALIVE_PERMIT_TIME_SECONDS,
TimeUnit.SECONDS)
Review Comment:
Non-blocking design question:
`permitKeepAliveTime`/`permitKeepAliveWithoutCalls` are set only inside `if
(CONNECT_GRPC_KEEPALIVE_ENABLED)`, but they're a different gRFC-A8 axis from
the two calls above them. `keepAliveTime`/`keepAliveTimeout` are the server's
*own* dead-client detection; `permitKeepAlive*` are the server's *tolerance of
client-initiated pings* — the same independence you cited when decoupling the
permit *value* in #3566748521, applied to the *gating* this time.
The client now defaults keepalive on unconditionally (60s pings,
without-calls, gated by a separate client flag). When an operator disables
*only* the server keepalive — which the PR advertises as an independent escape
hatch — the server drops back to grpc-netty's defaults (`permitKeepAliveTime` =
5 min, `permitKeepAliveWithoutCalls` = false; confirmed from the grpc-netty
1.76 `NettyServerBuilder` bytecode). A still-default-on client pinging every
60s then accrues ping-strikes and gets a `too_many_pings` GOAWAY on an
otherwise-healthy connection — the escape hatch triggering the very
false-positive teardowns it was meant to prevent. Same shape for a new 4.3.0
default-on client against an older / permit-unset server.
gRPC clients auto-back-off (double their keepAliveTime) on that GOAWAY, so
it's disruptive reconnects rather than a hard break — hence non-blocking. But
since the permit floor has no dependency on `keepAlive.time`, hoisting these
two calls out of the `if` (so the server always tolerates the pings its own
default-on clients send) is a small change that closes the gap. No test
currently exercises this cell — the disabled-path test disables both sides, so
a disabled-server / enabled-client case would cover it.
--
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]
