-
Notifications
You must be signed in to change notification settings - Fork 21
Improve log poll #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve log poll #103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the log polling mechanism by introducing asynchronous fetch request handling with a buffering system. The changes enable pipelined fetching where new fetch requests can be sent while the client processes previously fetched data, improving throughput and reducing latency.
Key Changes
- Introduced
LogFetchBufferto manage pending and completed fetches with async notification support - Refactored
LogFetcherto spawn async tasks for fetch requests, enabling non-blocking parallel fetching - Changed
LogRecordsBatchesfrom borrowed to owned data (usingBytes) to support cross-task data sharing - Added
is_from_remoteflag toReadContextto handle projection differences between local and remote log reads
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/fluss/src/client/table/log_fetch_buffer.rs | New file implementing buffering system for log fetches with traits for pending/completed fetches |
| crates/fluss/src/client/table/scanner.rs | Refactored poll() to use timeout-based retry logic; send_fetches() spawns async tasks for parallel fetching |
| crates/fluss/src/client/table/remote_log.rs | Modified RemoteLogDownloadFuture to support completion callbacks; RemotePendingFetch implements PendingFetch trait |
| crates/fluss/src/record/arrow.rs | Removed lifetimes from LogRecordsBatches/LogRecordBatch; added remote read support in ReadContext |
| crates/fluss/src/client/credentials.rs | CredentialsCache now owns RpcClient and Metadata for simplified async usage |
| crates/fluss/tests/integration/table_remote_scan.rs | Added projection test and increased timeout to accommodate async fetching delays |
| crates/fluss/tests/integration/table.rs | Increased poll timeouts for integration tests |
| crates/fluss/src/client/table/mod.rs | Added log_fetch_buffer module |
| crates/fluss/Cargo.toml | Added scopeguard dependency for cleanup guards |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@luoyuxia Thanks for the pr. Left minor comments from resource management perspective. PTAL |
58cd526 to
9653f81
Compare
|
@zhaohaidao @leekeiabstraction Thanks for your review. Comments addressed now. PTAL |
@LuoXia |
Purpose
Linked issue: close #94
Brief change log
Tests
API and Format
Documentation