You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "CuteChuanChuan (via GitHub)" <gi...@apache.org> on 2025/10/25 05:48:36 UTC
[PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
CuteChuanChuan opened a new pull request, #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337
# Which issue does this PR close?
Closes #117
# Rationale for this change
PR #115 introduced gRPC timeout configurations, but these values were hard-coded. This PR extends #115 by making all gRPC timeout values user-configurable through DataFusion's extensible configuration mechanism.
# What changes are included in this PR?
- Added 9 new configuration options to `BallistaConfig` (5 client-side, 4 server-side)
- Modified `create_grpc_client_connection()` and `create_grpc_server()` to accept config parameter
- Updated all call sites across executor and scheduler
# Are there any user-facing changes?
**Yes.** Users can now configure gRPC timeouts.
All existing tests pass.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2462723588
##########
ballista/core/src/utils.rs:
##########
@@ -106,31 +107,50 @@ pub async fn collect_stream(
pub async fn create_grpc_client_connection<D>(
dst: D,
+ config: &BallistaConfig,
Review Comment:
I derive `GrpcClientConfig` for this.
##########
ballista/core/src/utils.rs:
##########
@@ -106,31 +107,50 @@ pub async fn collect_stream(
pub async fn create_grpc_client_connection<D>(
dst: D,
+ config: &BallistaConfig,
) -> std::result::Result<Channel, Error>
where
D: std::convert::TryInto<tonic::transport::Endpoint>,
D::Error: Into<StdError>,
{
let endpoint = tonic::transport::Endpoint::new(dst)?
- .connect_timeout(Duration::from_secs(20))
- .timeout(Duration::from_secs(20))
+ .connect_timeout(Duration::from_secs(
+ config.default_grpc_client_connect_timeout_seconds() as u64,
+ ))
+ .timeout(Duration::from_secs(
+ config.default_grpc_client_timeout_seconds() as u64,
+ ))
// Disable Nagle's Algorithm since we don't want packets to wait
.tcp_nodelay(true)
- .tcp_keepalive(Option::Some(Duration::from_secs(3600)))
- .http2_keep_alive_interval(Duration::from_secs(300))
- .keep_alive_timeout(Duration::from_secs(20))
+ .tcp_keepalive(Option::Some(Duration::from_secs(
+ config.default_grpc_client_tcp_keepalive_seconds() as u64,
+ )))
+ .http2_keep_alive_interval(Duration::from_secs(
+ config.default_grpc_client_http2_keepalive_interval_seconds() as u64,
+ ))
+ .keep_alive_timeout(Duration::from_secs(
+ config.default_grpc_client_keepalive_timeout_seconds() as u64,
+ ))
.keep_alive_while_idle(true);
endpoint.connect().await
}
-pub fn create_grpc_server() -> Server {
+pub fn create_grpc_server(config: &BallistaConfig) -> Server {
Review Comment:
I derive `GrpcServerConfig` for this.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2478503004
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
+ pub connect_timeout_seconds: u64,
+ pub timeout_seconds: u64,
+ pub tcp_keepalive_seconds: u64,
+ pub http2_keepalive_interval_seconds: u64,
+ pub keepalive_timeout_seconds: u64,
+}
+
+impl GrpcClientConfig {
+ pub fn from_ballista_config(config: &BallistaConfig) -> Self {
Review Comment:
Considering that GrpcClientConfig is derived by extracting specific configs from BallistaConfig, I think `From` is more appropriate than `Into` here. WDYT?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2478517887
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
+ pub connect_timeout_seconds: u64,
+ pub timeout_seconds: u64,
+ pub tcp_keepalive_seconds: u64,
+ pub http2_keepalive_interval_seconds: u64,
+ pub keepalive_timeout_seconds: u64,
+}
+
+impl GrpcClientConfig {
+ pub fn from_ballista_config(config: &BallistaConfig) -> Self {
+ Self {
+ connect_timeout_seconds: config.default_grpc_client_connect_timeout_seconds()
+ as u64,
+ timeout_seconds: config.default_grpc_client_timeout_seconds() as u64,
+ tcp_keepalive_seconds: config.default_grpc_client_tcp_keepalive_seconds()
+ as u64,
+ http2_keepalive_interval_seconds: config
+ .default_grpc_client_http2_keepalive_interval_seconds()
+ as u64,
+ keepalive_timeout_seconds: config
+ .default_standalone_grpc_client_keepalive_timeout_seconds()
+ as u64,
+ }
+ }
+}
+
+impl Default for GrpcClientConfig {
+ fn default() -> Self {
+ Self {
+ connect_timeout_seconds: 20,
+ timeout_seconds: 20,
+ tcp_keepalive_seconds: 3600,
+ http2_keepalive_interval_seconds: 300,
+ keepalive_timeout_seconds: 20,
+ }
+ }
+}
+
+#[derive(Debug, Clone)]
+pub struct GrpcServerConfig {
Review Comment:
Absolutely!
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
Review Comment:
Absolutely!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2479210169
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
+ pub connect_timeout_seconds: u64,
+ pub timeout_seconds: u64,
+ pub tcp_keepalive_seconds: u64,
+ pub http2_keepalive_interval_seconds: u64,
+ pub keepalive_timeout_seconds: u64,
+}
+
+impl GrpcClientConfig {
+ pub fn from_ballista_config(config: &BallistaConfig) -> Self {
Review Comment:
`From` make sense, it will implement into as well, if i'm not mistaken
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3473842897
Fixed the unused imports. Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2462723469
##########
ballista/core/src/utils.rs:
##########
@@ -106,31 +107,50 @@ pub async fn collect_stream(
pub async fn create_grpc_client_connection<D>(
dst: D,
+ config: &BallistaConfig,
) -> std::result::Result<Channel, Error>
where
D: std::convert::TryInto<tonic::transport::Endpoint>,
D::Error: Into<StdError>,
{
let endpoint = tonic::transport::Endpoint::new(dst)?
- .connect_timeout(Duration::from_secs(20))
- .timeout(Duration::from_secs(20))
+ .connect_timeout(Duration::from_secs(
+ config.default_grpc_client_connect_timeout_seconds() as u64,
+ ))
+ .timeout(Duration::from_secs(
+ config.default_grpc_client_timeout_seconds() as u64,
+ ))
// Disable Nagle's Algorithm since we don't want packets to wait
.tcp_nodelay(true)
- .tcp_keepalive(Option::Some(Duration::from_secs(3600)))
- .http2_keep_alive_interval(Duration::from_secs(300))
- .keep_alive_timeout(Duration::from_secs(20))
+ .tcp_keepalive(Option::Some(Duration::from_secs(
+ config.default_grpc_client_tcp_keepalive_seconds() as u64,
+ )))
+ .http2_keep_alive_interval(Duration::from_secs(
+ config.default_grpc_client_http2_keepalive_interval_seconds() as u64,
+ ))
+ .keep_alive_timeout(Duration::from_secs(
+ config.default_grpc_client_keepalive_timeout_seconds() as u64,
+ ))
.keep_alive_while_idle(true);
endpoint.connect().await
}
-pub fn create_grpc_server() -> Server {
+pub fn create_grpc_server(config: &BallistaConfig) -> Server {
Review Comment:
I derive `GrpcClientConfig` and `GrpcServerConfig` for this.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3468823674
Hi @milenkovicm ,
Thanks for your comments and I really appreciate it. PTAL when you have a chance.
Thanks! 🙏
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2481768275
##########
ballista/executor/src/executor_process.rs:
##########
@@ -245,8 +249,11 @@ pub async fn start_executor_process(
));
let connect_timeout = opt.scheduler_connect_timeout_seconds as u64;
+ let session_config = (executor.config_producer)();
+ let ballista_config = session_config.ballista_config();
+ let grpc_client_config = GrpcClientConfig::from(&ballista_config);
let connection = if connect_timeout == 0 {
- create_grpc_client_connection(scheduler_url)
+ create_grpc_client_connection(scheduler_url, &grpc_client_config)
Review Comment:
Hi @milenkovicm ,
Good catch! However, since the `From` trait is implemented for `&BallistaConfig` (not `BallistaConfig`), I need `&(&ballista_config).into()` instead of `&ballista_config.into()`.
Would you prefer I change the implementation to `impl From<BallistaConfig>` so we can use the simpler `&ballista_config.into()` syntax?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3446565711
Hi @milenkovicm ,
Thanks for your comments and pointing out better approach. PTAL when you have a chance.
Thanks.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3455384676
apologies @CuteChuanChuan will try to review this change in next day or so
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3473567857
Hi @milenkovicm ,
I've addressed the feedback about simplifying the config usage and error found in Clippy. However, I noticed the CI is failing with rustdoc errors in `display.rs` and `execution_graph.rs`, which are files I haven't modified in this PR.
Would you like me to:
1. Fix them in this PR as well, or
2. Create a separate issue/PR for these rustdoc fixes?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2462614075
##########
ballista/core/src/execution_plans/distributed_query.rs:
##########
@@ -274,10 +275,11 @@ async fn execute_query(
session_id: String,
query: ExecuteQueryParams,
max_message_size: usize,
+ config: BallistaConfig,
Review Comment:
we already have gpc_max_message_size as a parameter can we pass other needed values as parameters rather than passing config
##########
ballista/executor/src/executor_server.rs:
##########
@@ -114,7 +117,7 @@ pub async fn startup<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan>(
let mut grpc_shutdown = shutdown_noti.subscribe_for_shutdown();
tokio::spawn(async move {
let shutdown_signal = grpc_shutdown.recv();
- let grpc_server_future = create_grpc_server()
+ let grpc_server_future = create_grpc_server(&ballista_config)
Review Comment:
this configuration should be provided as part of executor binary configuration, not ballista condif
##########
ballista/core/src/execution_plans/distributed_query.rs:
##########
@@ -238,6 +238,7 @@ impl<T: 'static + AsLogicalPlan> ExecutionPlan for DistributedQueryExec<T> {
self.session_id.clone(),
query,
self.config.default_grpc_client_max_message_size(),
+ self.config.clone(),
Review Comment:
we already have `gpc_max_message_size` as a parameter can we pass other needed values as parameters rather than passing config
##########
ballista/core/src/utils.rs:
##########
@@ -106,31 +107,50 @@ pub async fn collect_stream(
pub async fn create_grpc_client_connection<D>(
dst: D,
+ config: &BallistaConfig,
Review Comment:
as i mentioned before, IMHO it makes more sense to pass individual parameters, or we can derive some kind of `GprcConfig` if there are too many of them
##########
ballista/core/src/utils.rs:
##########
@@ -106,31 +107,50 @@ pub async fn collect_stream(
pub async fn create_grpc_client_connection<D>(
dst: D,
+ config: &BallistaConfig,
) -> std::result::Result<Channel, Error>
where
D: std::convert::TryInto<tonic::transport::Endpoint>,
D::Error: Into<StdError>,
{
let endpoint = tonic::transport::Endpoint::new(dst)?
- .connect_timeout(Duration::from_secs(20))
- .timeout(Duration::from_secs(20))
+ .connect_timeout(Duration::from_secs(
+ config.default_grpc_client_connect_timeout_seconds() as u64,
+ ))
+ .timeout(Duration::from_secs(
+ config.default_grpc_client_timeout_seconds() as u64,
+ ))
// Disable Nagle's Algorithm since we don't want packets to wait
.tcp_nodelay(true)
- .tcp_keepalive(Option::Some(Duration::from_secs(3600)))
- .http2_keep_alive_interval(Duration::from_secs(300))
- .keep_alive_timeout(Duration::from_secs(20))
+ .tcp_keepalive(Option::Some(Duration::from_secs(
+ config.default_grpc_client_tcp_keepalive_seconds() as u64,
+ )))
+ .http2_keep_alive_interval(Duration::from_secs(
+ config.default_grpc_client_http2_keepalive_interval_seconds() as u64,
+ ))
+ .keep_alive_timeout(Duration::from_secs(
+ config.default_grpc_client_keepalive_timeout_seconds() as u64,
+ ))
.keep_alive_while_idle(true);
endpoint.connect().await
}
-pub fn create_grpc_server() -> Server {
+pub fn create_grpc_server(config: &BallistaConfig) -> Server {
Review Comment:
as i mentioned before, IMHO it makes more sense to pass individual parameters, or we can derive some kind of GprcConfig if there are too many of them
##########
ballista/core/src/config.rs:
##########
@@ -39,6 +39,28 @@ pub const BALLISTA_SHUFFLE_READER_FORCE_REMOTE_READ: &str =
pub const BALLISTA_SHUFFLE_READER_REMOTE_PREFER_FLIGHT: &str =
"ballista.shuffle.remote_read_prefer_flight";
+// gRPC client timeout configurations
+pub const BALLISTA_GRPC_CLIENT_CONNECT_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.connect_timeout_seconds";
+pub const BALLISTA_GRPC_CLIENT_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.timeout_seconds";
+pub const BALLISTA_GRPC_CLIENT_TCP_KEEPALIVE_SECONDS: &str =
+ "ballista.grpc.client.tcp_keepalive_seconds";
+pub const BALLISTA_GRPC_CLIENT_HTTP2_KEEPALIVE_INTERVAL_SECONDS: &str =
+ "ballista.grpc.client.http2_keepalive_interval_seconds";
+pub const BALLISTA_GRPC_CLIENT_KEEPALIVE_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.keepalive_timeout_seconds";
+
+// gRPC server timeout configurations
Review Comment:
this config is relevant to `standalone` version only can we prefix it somehow to identify it as standalone option?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3473655696
should be fixed on main, rebase your branch
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm merged PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3476837442
thanks on your contribution @CuteChuanChuan
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2478449413
##########
ballista/core/src/config.rs:
##########
@@ -39,6 +39,18 @@ pub const BALLISTA_SHUFFLE_READER_FORCE_REMOTE_READ: &str =
pub const BALLISTA_SHUFFLE_READER_REMOTE_PREFER_FLIGHT: &str =
"ballista.shuffle.remote_read_prefer_flight";
+// gRPC client timeout configurations
+pub const BALLISTA_GRPC_CLIENT_CONNECT_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.connect_timeout_seconds";
+pub const BALLISTA_GRPC_CLIENT_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.timeout_seconds";
+pub const BALLISTA_GRPC_CLIENT_TCP_KEEPALIVE_SECONDS: &str =
+ "ballista.grpc.client.tcp_keepalive_seconds";
+pub const BALLISTA_GRPC_CLIENT_HTTP2_KEEPALIVE_INTERVAL_SECONDS: &str =
+ "ballista.grpc.client.http2_keepalive_interval_seconds";
+pub const BALLISTA_STANDALONE_GRPC_CLIENT_KEEPALIVE_TIMEOUT_SECONDS: &str =
Review Comment:
Thanks for pointing this out. You're right. We can make configuration options simpler.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "CuteChuanChuan (via GitHub)" <gi...@apache.org>.
CuteChuanChuan commented on PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#issuecomment-3457313714
No worries at all! Take your time - I appreciate you taking a look at this. 🙏
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2479219494
##########
ballista/executor/src/executor_server.rs:
##########
@@ -235,7 +239,11 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan> ExecutorServer<T,
Ok(scheduler)
} else {
let scheduler_url = format!("http://{scheduler_id}");
- let connection = create_grpc_client_connection(scheduler_url).await?;
+ let session_config = (self.executor.config_producer)();
+ let ballista_config = session_config.ballista_config();
+ let grpc_client_config = GrpcClientConfig::from(&ballista_config);
+ let connection =
+ create_grpc_client_connection(scheduler_url, &grpc_client_config).await?;
Review Comment:
could we have
```rust
create_grpc_client_connection(scheduler_url, &ballista_config.into())
```
it would remove
```rust
let grpc_client_config = GrpcClientConfig::from(&ballista_config);
```
making it a bit easier to read?
##########
ballista/executor/src/executor_process.rs:
##########
@@ -245,8 +249,11 @@ pub async fn start_executor_process(
));
let connect_timeout = opt.scheduler_connect_timeout_seconds as u64;
+ let session_config = (executor.config_producer)();
+ let ballista_config = session_config.ballista_config();
+ let grpc_client_config = GrpcClientConfig::from(&ballista_config);
let connection = if connect_timeout == 0 {
- create_grpc_client_connection(scheduler_url)
+ create_grpc_client_connection(scheduler_url, &grpc_client_config)
Review Comment:
could we have
```rust
create_grpc_client_connection(scheduler_url, &ballista_config.into())
```
it would remove
```rust
let grpc_client_config = GrpcClientConfig::from(&ballista_config);
```
making it a bit easier to read?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: make gRPC timeout configurations user-configurable [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1337:
URL: https://github.com/apache/datafusion-ballista/pull/1337#discussion_r2475452402
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
+ pub connect_timeout_seconds: u64,
+ pub timeout_seconds: u64,
+ pub tcp_keepalive_seconds: u64,
+ pub http2_keepalive_interval_seconds: u64,
+ pub keepalive_timeout_seconds: u64,
+}
+
+impl GrpcClientConfig {
+ pub fn from_ballista_config(config: &BallistaConfig) -> Self {
Review Comment:
would it make sense to implement this as `From` or `Into` not sure which one would be better
##########
ballista/scheduler/src/state/executor_manager.rs:
##########
@@ -427,7 +429,9 @@ impl ExecutorManager {
"http://{}:{}",
executor_metadata.host, executor_metadata.grpc_port
);
- let connection = create_grpc_client_connection(executor_url).await?;
+ let grpc_config = GrpcClientConfig::default();
Review Comment:
can this be method parameter, so we can use `BallistaConfig` value instead of default values here?
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
+ pub connect_timeout_seconds: u64,
+ pub timeout_seconds: u64,
+ pub tcp_keepalive_seconds: u64,
+ pub http2_keepalive_interval_seconds: u64,
+ pub keepalive_timeout_seconds: u64,
+}
+
+impl GrpcClientConfig {
+ pub fn from_ballista_config(config: &BallistaConfig) -> Self {
+ Self {
+ connect_timeout_seconds: config.default_grpc_client_connect_timeout_seconds()
+ as u64,
+ timeout_seconds: config.default_grpc_client_timeout_seconds() as u64,
+ tcp_keepalive_seconds: config.default_grpc_client_tcp_keepalive_seconds()
+ as u64,
+ http2_keepalive_interval_seconds: config
+ .default_grpc_client_http2_keepalive_interval_seconds()
+ as u64,
+ keepalive_timeout_seconds: config
+ .default_standalone_grpc_client_keepalive_timeout_seconds()
+ as u64,
+ }
+ }
+}
+
+impl Default for GrpcClientConfig {
+ fn default() -> Self {
+ Self {
+ connect_timeout_seconds: 20,
+ timeout_seconds: 20,
+ tcp_keepalive_seconds: 3600,
+ http2_keepalive_interval_seconds: 300,
+ keepalive_timeout_seconds: 20,
+ }
+ }
+}
+
+#[derive(Debug, Clone)]
+pub struct GrpcServerConfig {
Review Comment:
can you please put some docs please, we will try to catch up with documentation at some point so it would help.
##########
ballista/core/src/config.rs:
##########
@@ -39,6 +39,18 @@ pub const BALLISTA_SHUFFLE_READER_FORCE_REMOTE_READ: &str =
pub const BALLISTA_SHUFFLE_READER_REMOTE_PREFER_FLIGHT: &str =
"ballista.shuffle.remote_read_prefer_flight";
+// gRPC client timeout configurations
+pub const BALLISTA_GRPC_CLIENT_CONNECT_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.connect_timeout_seconds";
+pub const BALLISTA_GRPC_CLIENT_TIMEOUT_SECONDS: &str =
+ "ballista.grpc.client.timeout_seconds";
+pub const BALLISTA_GRPC_CLIENT_TCP_KEEPALIVE_SECONDS: &str =
+ "ballista.grpc.client.tcp_keepalive_seconds";
+pub const BALLISTA_GRPC_CLIENT_HTTP2_KEEPALIVE_INTERVAL_SECONDS: &str =
+ "ballista.grpc.client.http2_keepalive_interval_seconds";
+pub const BALLISTA_STANDALONE_GRPC_CLIENT_KEEPALIVE_TIMEOUT_SECONDS: &str =
Review Comment:
Do we need standalone config? Standalone is used just for local testing, maybe if we can live without it. It would make configuration options simpler. WDYT?
##########
ballista/core/src/utils.rs:
##########
@@ -36,6 +37,64 @@ use std::{fs::File, pin::Pin};
use tonic::codegen::StdError;
use tonic::transport::{Channel, Error, Server};
+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {
Review Comment:
can you please put some docs please, we will try to catch up with documentation at some point so it would help.
##########
ballista/scheduler/src/standalone.rs:
##########
@@ -92,8 +93,9 @@ pub async fn new_standalone_scheduler_with_builder(
let listener = TcpListener::bind("localhost:0").await?;
let addr = listener.local_addr()?;
info!("Ballista v{BALLISTA_VERSION} Rust Scheduler listening on {addr:?}");
+ let grpc_server_config = GrpcServerConfig::default();
tokio::spawn(
- create_grpc_server()
+ create_grpc_server(&grpc_server_config)
Review Comment:
I would suggest
```rust
create_grpc_server(&GrpcServerConfig::default();)
```
if possible
##########
ballista/executor/src/standalone.rs:
##########
@@ -123,8 +123,9 @@ pub async fn new_standalone_executor_from_builder(
.max_decoding_message_size(max_message_size)
.max_encoding_message_size(max_message_size);
+ let grpc_server_config = GrpcServerConfig::default();
Review Comment:
can you avoid variable here and create config at method call
```rust
create_grpc_server(&GrpcServerConfig::default();)
```
##########
ballista/client/tests/context_checks.rs:
##########
@@ -240,12 +240,12 @@ mod supported {
.await?;
let expected = [
- "+---------------------------------------+----------+",
- "| name | value |",
- "+---------------------------------------+----------+",
- "| ballista.grpc_client_max_message_size | 16777216 |",
- "| ballista.job.name | |",
- "+---------------------------------------+----------+",
+ "+-------------------------------------------------------+-------+",
+ "| name | value |",
+ "+-------------------------------------------------------+-------+",
Review Comment:
can we please update query to return ` ballista.job.name` only. so we don't change the test every time we add new option.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org