feat: Add request timeout for rpc#399
Conversation
There was a problem hiding this comment.
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_msconfiguration 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.
fresh-borzoni
left a comment
There was a problem hiding this comment.
@charlesdong1991 Ty for the PR, left comments, PTAL
|
I addressed your comments, thanks again. @fresh-borzoni PTAL |
fresh-borzoni
left a comment
There was a problem hiding this comment.
@charlesdong1991 Ty, left small comment and also rust docs are missing
fresh-borzoni
left a comment
There was a problem hiding this comment.
@charlesdong1991 TY, LGTM
Purpose
Linked issue: close #398
Brief change log
Tests
Tests all pass locally.
API and Format
Documentation