Skip to content

feat: Add request timeout for rpc#399

Open
charlesdong1991 wants to merge 8 commits intoapache:mainfrom
charlesdong1991:add-per-request
Open

feat: Add request timeout for rpc#399
charlesdong1991 wants to merge 8 commits intoapache:mainfrom
charlesdong1991:add-per-request

Conversation

@charlesdong1991
Copy link
Contributor

Purpose

Linked issue: close #398

Brief change log

  • Add request_timout_ms config
  • Add a new request timeout error variant
  • Propagate new config to python and c++ bindings

Tests

Tests all pass locally.

API and Format

Documentation

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a configurable per-RPC request timeout to prevent client calls from hanging indefinitely when the server stops responding (closes #398), and threads the new setting through Rust + Python/C++ bindings.

Changes:

  • Introduce request_timeout_ms configuration with a 30s default.
  • Apply request-level timeout in the RPC client and add RpcError::RequestTimeout.
  • Expose the new config in Python and C++ bindings.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
crates/fluss/src/rpc/server_connection.rs Adds request-timeout handling around response await; includes new unit tests.
crates/fluss/src/rpc/error.rs Adds RequestTimeout RPC error variant.
crates/fluss/src/config.rs Adds request_timeout_ms config with default and debug/default wiring.
crates/fluss/src/client/connection.rs Plumbs request_timeout_ms into RpcClient construction.
bindings/python/src/config.rs Parses and exposes request_timeout_ms in Python binding config.
bindings/python/fluss/__init__.pyi Adds Config.request_timeout_ms typing to Python stubs.
bindings/cpp/src/lib.rs Extends C++ FFI config to include request_timeout_ms.
bindings/cpp/src/ffi_converter.hpp Converts C++ Configuration.request_timeout_ms into FFI config.
bindings/cpp/include/fluss.hpp Exposes request_timeout_ms on the public C++ Configuration struct.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@fresh-borzoni fresh-borzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlesdong1991 Ty for the PR, left comments, PTAL

@charlesdong1991
Copy link
Contributor Author

I addressed your comments, thanks again. @fresh-borzoni PTAL

Copy link
Contributor

@fresh-borzoni fresh-borzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlesdong1991 Ty, left small comment and also rust docs are missing

Copy link
Contributor

@fresh-borzoni fresh-borzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlesdong1991 TY, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add per-request timeout for rpc layer

3 participants