Conversation
f802791 to
9e9d831
Compare
|
Hi, @luoyuxia @leekeiabstraction Please take a look if you have time. |
There was a problem hiding this comment.
Pull request overview
This pull request adds batch lookup support to the Rust client, achieving parity with the Java client by implementing a queuing and batching mechanism that reduces network round trips for improved throughput.
Changes:
- Adds a
LookupClientwith background sender task that batches multiple lookup operations - Implements retry logic with configurable limits and error classification
- Adds five new configuration parameters for lookup batching, queuing, and retry behavior
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/fluss/src/config.rs | Adds configuration parameters for lookup queue size, batch size, timeout, inflight requests, and max retries |
| crates/fluss/src/rpc/message/lookup.rs | Adds new_batched method to create lookup requests with multiple buckets |
| crates/fluss/src/rpc/fluss_api_error.rs | Adds is_retriable() method to classify errors for retry logic |
| crates/fluss/src/client/connection.rs | Adds get_or_create_lookup_client() to manage shared lookup client instance |
| crates/fluss/src/client/mod.rs | Exports new lookup module and LookupClient |
| crates/fluss/src/client/lookup/mod.rs | Defines lookup module structure with client, query, queue, and sender |
| crates/fluss/src/client/lookup/lookup_query.rs | Represents individual lookup operation with retry tracking and oneshot channel for result |
| crates/fluss/src/client/lookup/lookup_queue.rs | Implements queue with batch draining and retry prioritization |
| crates/fluss/src/client/lookup/lookup_sender.rs | Background task that groups lookups by destination, batches requests, and handles responses/retries |
| crates/fluss/src/client/lookup/lookup_client.rs | Public API for batched lookups with background sender task management |
| crates/fluss/src/client/table/lookup.rs | Updates Lookuper to use shared LookupClient instead of direct RPC calls |
| crates/fluss/tests/integration/kv_table.rs | Adds integration tests for batched lookups with various scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leekeiabstraction
left a comment
There was a problem hiding this comment.
Thank you for the PR, left initial comments
|
Also, consider squashing the integration test cases into 1 test case. Concurrent lookups, looking up same key, looking up non-existing key should be able to be rolled into one. I'm starting to think that we may also want to add number of tablet servers for integration test so that we can check the behaviour of bucketing as well. What do you think @luoyuxia . We can potentially raise that as a separate issue. |
leekeiabstraction
left a comment
There was a problem hiding this comment.
Thank you for the update! One comment left from me.
leekeiabstraction
left a comment
There was a problem hiding this comment.
Approved and thank you for the PR! Left a small comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@linguoxuan Hi, thanks for your contribution. But since we're getting to code freeze for 0.1. I'd like to get it merged int next version. Does it work for you? |
ok. |
Purpose
Linked issue: close #195
Brief change log
Tests
API and Format
Documentation