andygrove commented on code in PR #2816:
URL:
https://github.com/apache/datafusion-ballista/pull/2816#discussion_r4531423800
##########
ballista/scheduler/src/scheduler_process.rs:
##########
@@ -258,20 +258,35 @@ 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);
Review Comment:
Small nit. This local shadows the `tcp_keepalive` function it calls. It
works fine, but something like `keepalive` might read a little clearer.
##########
ballista/core/src/utils.rs:
##########
@@ -452,6 +452,28 @@ where
}
}
+/// Converts a TCP keepalive interval in seconds to the idle time passed to
+/// tonic and `socket2`. A value of 0 disables keepalive.
+pub fn tcp_keepalive(seconds: u64) -> Option<Duration> {
Review Comment:
Treating `0` as disabled makes sense here. The client builders in this file
(`create_grpc_client_connection` and `create_grpc_client_endpoint`) still pass
`Some(Duration::from_secs(config.tcp_keepalive_seconds))`, though. So
`ballista.client.grpc_tcp_keepalive_seconds = 0` still behaves the old way.
Would it make sense to use `tcp_keepalive` there as well? A follow-up issue is
fine too if you'd rather keep this PR small.
--
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]
