andygrove commented on code in PR #2816:
URL:
https://github.com/apache/datafusion-ballista/pull/2816#discussion_r4525129772
##########
ballista/scheduler/src/standalone.rs:
##########
@@ -120,11 +121,18 @@ pub async fn new_standalone_scheduler_with_builder(
info!(
"Ballista Scheduler v{BALLISTA_VERSION} (DataFusion
v{DATAFUSION_VERSION}) listening on {addr:?}"
);
+ let grpc_server_config = GrpcServerConfig::default();
+ let incoming = TcpIncoming::from(listener)
Review Comment:
Good catch that tonic ignores `tcp_keepalive` on a pre-bound listener here
too. This now duplicates the `with_nodelay`/`with_keepalive` chain in
`create_grpc_server_incoming` in `ballista/core/src/utils.rs`. Could you pull
that into a small helper in `ballista_core::utils` that takes a `TcpListener`
and a `&GrpcServerConfig`? Then `create_grpc_server_incoming` and this call
site can both use it and won't drift apart.
##########
ballista/scheduler/src/scheduler_process.rs:
##########
@@ -258,20 +258,41 @@ pub async fn start_grpc_service_with_listener<
let final_route =
with_version_headers(merged).into_make_service_with_connect_info::<SocketAddr>();
- // axum, unlike tonic's server, leaves Nagle on, which delays the later
- // HTTP/2 frames of each poll response behind the client's delayed ACK
- // (~40 ms on Linux).
- let listener = listener.tap_io(|tcp| {
- if let Err(e) = tcp.set_nodelay(true) {
- warn!("Failed to set TCP_NODELAY on a scheduler connection: {e}");
- }
- });
+ let tcp_keepalive =
tcp_keepalive(config.grpc_server_tcp_keepalive_seconds);
+ let listener = listener.tap_io(move |tcp| configure_connection(tcp,
tcp_keepalive));
axum::serve(listener, final_route)
.with_graceful_shutdown(async move { shutdown.recv().await })
.await
.map_err(BallistaError::from)
}
+/// Converts a keepalive setting in seconds into the idle time passed to
+/// [`configure_connection`], where 0 disables TCP keepalive.
+fn tcp_keepalive(seconds: u64) -> Option<Duration> {
Review Comment:
Here `0` disables keepalive, but `GrpcServerConfig::tcp_keepalive_seconds =
0` still passes `Some(Duration::from_secs(0))` to `with_keepalive` in
`create_grpc_server_incoming`. Could you make the two agree, so `0` means
disabled everywhere? Reusing this `tcp_keepalive` conversion in core (alongside
the helper from the other comment) 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]
