feat: make gRPC timeout configurations user-configurable #1337

CuteChuanChuan · 2025-10-25T05:48:33Z

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.

- Extends PR apache#115 by making gRPC timeout values configurable instead of hard-coded.

ballista/core/src/execution_plans/distributed_query.rs

ballista/core/src/config.rs

milenkovicm · 2025-10-25T06:46:53Z

ballista/executor/src/executor_server.rs

        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)


this configuration should be provided as part of executor binary configuration, not ballista condif

ballista/core/src/utils.rs

- Add GrpcClientConfig and GrpcServerConfig structs - Update create_grpc_client_connection/create_grpc_server signatures - Move server configs to ExecutorProcessConfig - Add standalone prefix to keepalive_timeout config

CuteChuanChuan · 2025-10-25T11:15:54Z

Hi @milenkovicm ,
Thanks for your comments and pointing out better approach. PTAL when you have a chance.
Thanks.

milenkovicm · 2025-10-28T09:18:11Z

apologies @CuteChuanChuan will try to review this change in next day or so

CuteChuanChuan · 2025-10-28T16:13:07Z

No worries at all! Take your time - I appreciate you taking a look at this. 🙏

milenkovicm

Firs of all sorry for late review, and thank you for your contribution !
I think changes make sense, I have just few minor comments, please have a look when you get chance

milenkovicm · 2025-10-29T20:54:08Z

ballista/client/tests/context_checks.rs

-            "+---------------------------------------+----------+",
+            "+-------------------------------------------------------+-------+",
+            "| name                                                  | value |",
+            "+-------------------------------------------------------+-------+",


can we please update query to return ballista.job.name only. so we don't change the test every time we add new option.

milenkovicm · 2025-10-29T20:55:39Z

ballista/core/src/config.rs

+    "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 =


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?

Thanks for pointing this out. You're right. We can make configuration options simpler.

milenkovicm · 2025-10-29T20:59:14Z

ballista/core/src/utils.rs

+}
+
+impl GrpcClientConfig {
+    pub fn from_ballista_config(config: &BallistaConfig) -> Self {


would it make sense to implement this as From or Into not sure which one would be better

Considering that GrpcClientConfig is derived by extracting specific configs from BallistaConfig, I think From is more appropriate than Into here. WDYT?

From make sense, it will implement into as well, if i'm not mistaken

milenkovicm · 2025-10-29T21:00:35Z

ballista/core/src/utils.rs

+}
+
+#[derive(Debug, Clone)]
+pub struct GrpcServerConfig {


can you please put some docs please, we will try to catch up with documentation at some point so it would help.

Absolutely!

milenkovicm · 2025-10-29T21:00:41Z

ballista/core/src/utils.rs

 use tonic::transport::{Channel, Error, Server};

+#[derive(Debug, Clone)]
+pub struct GrpcClientConfig {


can you please put some docs please, we will try to catch up with documentation at some point so it would help.

Absolutely!

milenkovicm · 2025-10-29T21:02:08Z

ballista/executor/src/standalone.rs

        .max_decoding_message_size(max_message_size)
        .max_encoding_message_size(max_message_size);

+    let grpc_server_config = GrpcServerConfig::default();


can you avoid variable here and create config at method call

create_grpc_server(&GrpcServerConfig::default();)

milenkovicm · 2025-10-29T21:03:51Z

ballista/scheduler/src/state/executor_manager.rs

                executor_metadata.host, executor_metadata.grpc_port
            );
-            let connection = create_grpc_client_connection(executor_url).await?;
+            let grpc_config = GrpcClientConfig::default();


can this be method parameter, so we can use BallistaConfig value instead of default values here?

milenkovicm · 2025-10-29T21:04:35Z

ballista/scheduler/src/standalone.rs

+    let grpc_server_config = GrpcServerConfig::default();
    tokio::spawn(
-        create_grpc_server()
+        create_grpc_server(&grpc_server_config)


I would suggest

create_grpc_server(&GrpcServerConfig::default();)

if possible

- Simplify config test to query only ballista.job.name - Remove standalone-specific gRPC configurations - Implement From trait for GrpcClientConfig conversion - Add documentation to gRPC config structs - Inline GrpcServerConfig creation in executor and scheduler - Pass BallistaConfig to ExecutorManager for proper config propagation

CuteChuanChuan · 2025-10-30T16:13:59Z

Hi @milenkovicm ,
Thanks for your comments and I really appreciate it. PTAL when you have a chance.
Thanks! 🙏

milenkovicm

i have just two small coments, if you agree

milenkovicm · 2025-10-30T19:04:23Z

ballista/executor/src/executor_process.rs

+    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)


could we have

create_grpc_client_connection(scheduler_url, &ballista_config.into())

it would remove

let grpc_client_config = GrpcClientConfig::from(&ballista_config);

making it a bit easier to read?

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?

milenkovicm · 2025-10-30T19:04:40Z

ballista/executor/src/executor_server.rs

+            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?;


could we have

create_grpc_client_connection(scheduler_url, &ballista_config.into())

it would remove

let grpc_client_config = GrpcClientConfig::from(&ballista_config);

making it a bit easier to read?

CuteChuanChuan · 2025-10-31T15:21:03Z

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:

Fix them in this PR as well, or
Create a separate issue/PR for these rustdoc fixes?

- Use Into trait conversion for GrpcClientConfig creation - Fix clippy needless_borrow warning in executor_manager

milenkovicm · 2025-10-31T15:40:03Z

should be fixed on main, rebase your branch

CuteChuanChuan · 2025-10-31T16:21:21Z

Fixed the unused imports. Thanks!

milenkovicm · 2025-11-01T20:55:46Z

thanks on your contribution @CuteChuanChuan

feat: make gRPC timeout configurations user-configurable

384c4ec

- Extends PR apache#115 by making gRPC timeout values configurable instead of hard-coded.

milenkovicm reviewed Oct 25, 2025

View reviewed changes

refactor: introduce GrpcClientConfig and GrpcServerConfig

23cd355

- Add GrpcClientConfig and GrpcServerConfig structs - Update create_grpc_client_connection/create_grpc_server signatures - Move server configs to ExecutorProcessConfig - Add standalone prefix to keepalive_timeout config

milenkovicm reviewed Oct 29, 2025

View reviewed changes

milenkovicm approved these changes Oct 30, 2025

View reviewed changes

refactor: simplify GrpcClientConfig usage with Into trait

c6b8d87

- Use Into trait conversion for GrpcClientConfig creation - Fix clippy needless_borrow warning in executor_manager

CuteChuanChuan and others added 3 commits October 31, 2025 23:43

Cargo fmt

3d9f9f0

Merge branch 'apache:main' into cutechuanchuan/grpc-user-configurable

10ab817

fix: remove unused GrpcClientConfig imports

c83bc84

milenkovicm merged commit 917741a into apache:main Nov 1, 2025
27 checks passed

CuteChuanChuan deleted the cutechuanchuan/grpc-user-configurable branch November 2, 2025 02:20

feat: make gRPC timeout configurations user-configurable #1337

feat: make gRPC timeout configurations user-configurable #1337

Uh oh!

Conversation

CuteChuanChuan commented Oct 25, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CuteChuanChuan commented Oct 25, 2025

Uh oh!

milenkovicm commented Oct 28, 2025

Uh oh!

CuteChuanChuan commented Oct 28, 2025

Uh oh!

milenkovicm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CuteChuanChuan commented Oct 30, 2025

Uh oh!

milenkovicm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CuteChuanChuan commented Oct 31, 2025

Uh oh!

milenkovicm commented Oct 31, 2025

Uh oh!

CuteChuanChuan commented Oct 31, 2025

Uh oh!

milenkovicm commented Nov 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants