chore: remove GrpcQueryHandler::put_record_batch#7844
chore: remove GrpcQueryHandler::put_record_batch#7844killme2008 merged 1 commit intoGreptimeTeam:mainfrom
Conversation
…ueryHandler::handle_put_record_batch_stream instead Signed-off-by: Lei, HUANG <mrsatangel@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on refining the gRPC query handling interface by eliminating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request removes the unused GrpcQueryHandler::put_record_batch method and its implementations. The changes are straightforward and correctly remove the dead code from the GrpcQueryHandler trait, its implementation in Instance, and the corresponding test mock. This cleanup simplifies the codebase by removing an unused code path for non-streaming batch puts via gRPC.
There was a problem hiding this comment.
Pull request overview
Removes the now-unneeded GrpcQueryHandler::put_record_batch API in favor of the streaming-based bulk insert path, and updates implementations accordingly.
Changes:
- Removed
put_record_batchfrom theGrpcQueryHandlertrait. - Dropped the corresponding (previously
unimplemented!()/ unused) implementations in frontend and test dummy handler. - Cleaned up now-unused imports (
AffectedRows,TableRef,PutRecordBatchRequest).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/servers/src/query_handler/grpc.rs | Removes put_record_batch from the public GrpcQueryHandler trait and related imports. |
| src/frontend/src/instance/grpc.rs | Removes the put_record_batch impl from Instance; relies on handle_put_record_batch_stream. |
| src/servers/tests/mod.rs | Removes the dummy put_record_batch impl and AffectedRows import in tests. |
Comments suppressed due to low confidence (1)
src/servers/src/query_handler/grpc.rs:40
- Removing
put_record_batchfrom the publicGrpcQueryHandlertrait is a breaking API change for any downstream implementors of theserverscrate. If this trait is intended to be part of the public API, the PR checklist item "API changes are backward compatible" should be updated (or the trait should be made non-public / sealed to indicate it’s internal).
#[async_trait]
pub trait GrpcQueryHandler {
async fn do_query(&self, query: Request, ctx: QueryContextRef) -> Result<Output>;
fn handle_put_record_batch_stream(
&self,
stream: PutRecordBatchRequestStream,
ctx: QueryContextRef,
) -> Pin<Box<dyn Stream<Item = Result<DoPutResponse>> + Send>>;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
As title
PR Checklist
Please convert it to a draft if some of the following conditions are not met.