vinodkc commented on code in PR #57202:
URL: https://github.com/apache/spark/pull/57202#discussion_r3581243460
##########
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:
Fixed in f5820e8cf06: hoisted
`permitKeepAliveTime`/`permitKeepAliveWithoutCalls` out of the `if
(CONNECT_GRPC_KEEPALIVE_ENABLED)` gate so they're applied unconditionally,
matching the gRFC A8 independence you flagged. Added a regression test
(`SparkConnectServiceKeepAliveSuite`, "server tolerates a default-on client's
PINGs even with its own keepalive detection disabled") that starts the real
service with server keepalive disabled, makes one call to establish the
transport, then goes genuinely idle for several client ping intervals, and
asserts no new TCP connection is needed afterward. Verified it actually catches
the regression: reverting just this hoist makes the test fail (connection count
1 -> 2, confirming a forceful too_many_pings close and reconnect), and pass
again with the fix.
--
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]
