refactor: ClientRequestProcessor#process_request #4033
Conversation
WalkthroughThe client request handling in ClientRequestProcessor was simplified by removing the process_request_inner helper and delegating directly to get_route_info_by_topic within process_request. Parameter names in process_request were prefixed with underscores to indicate they are unused. No behavioral changes to get_route_info_by_topic were made. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Namesrv as ClientRequestProcessor
participant Router as get_route_info_by_topic
Client->>Namesrv: process_request(request, _channel, _ctx)
Note right of Namesrv: Parameters _channel/_ctx are unused
Namesrv->>Router: get_route_info_by_topic(request)
Router-->>Namesrv: RouteInfo / Response
Namesrv-->>Client: Response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
rocketmq-rust-robot
added
Difficulty level/Easy
|
🔊@Parizval 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4033 +/- ##
=======================================
Coverage 26.52% 26.52%
=======================================
Files 565 565
Lines 80779 80770 -9
=======================================
Hits 21426 21426
+ Misses 59353 59344 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-namesrv/src/processor/client_request_processor.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-namesrv/src/processor/client_request_processor.rs (1)
rocketmq-remoting/src/code/request_code.rs (2)
from(197-199)from(203-368)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-namesrv/src/processor/client_request_processor.rs (1)
49-51: Underscore-prefixed params: good call to silence unused warnings.No behavior change; matches trait signature while documenting non-use.
|
|
||
| self.get_route_info_by_topic(request) | ||
| } |
There was a problem hiding this comment.
Don’t unconditionally route every request to get_route_info_by_topic — this breaks non-105 request handling.
As written, all request codes attempt to decode GetRouteInfoRequestHeader, which will error for other codes and can surface as protocol failures. Restore dispatch on request_code and return a proper “not supported” response for others.
Apply this diff:
- self.get_route_info_by_topic(request)
+ match request_code {
+ RequestCode::GetRouteinfoByTopic => self.get_route_info_by_topic(request),
+ other => {
+ warn!("Unsupported request code: {:?}.", other);
+ Ok(Some(
+ RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::RequestCodeNotSupported,
+ )
+ .set_remark(format!("Unsupported request code: {:?}", other)),
+ ))
+ }
+ }If RemotingSysResponseCode::RequestCodeNotSupported doesn’t exist in this codebase, fall back to RemotingSysResponseCode::SystemError with a clear remark.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.get_route_info_by_topic(request) | |
| } | |
| match request_code { | |
| RequestCode::GetRouteInfoByTopic => self.get_route_info_by_topic(request), | |
| other => { | |
| warn!("Unsupported request code: {:?}.", other); | |
| Ok(Some( | |
| RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError, | |
| ) | |
| .set_remark(format!("Unsupported request code: {:?}", other)), | |
| )) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In rocketmq-namesrv/src/processor/client_request_processor.rs around lines 58 to
60, the current code unconditionally calls get_route_info_by_topic(request)
causing every request to be decoded as GetRouteInfoRequestHeader which fails for
non-105 request codes; restore proper dispatch by matching on
request.request_code (or equivalent) and only call get_route_info_by_topic for
the GetRouteInfo request code (105), and for all other codes return an immediate
RemotingCommand response with RemotingSysResponseCode::RequestCodeNotSupported
(or if that variant is absent, use RemotingSysResponseCode::SystemError) and set
a clear remark stating the request code is not supported to avoid protocol
decode errors.
rocketmq-rust-bot
added
approved
4 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4022
Brief Description
Functional call is refactored to call function get_route_info_by_topic directly
How Did You Test This Change?
Summary by CodeRabbit