implement handle_get_next_broker_id for broker ID allocation#5683
implement handle_get_next_broker_id for broker ID allocation#5683
Conversation
WalkthroughImplemented the controller's ControllerGetNextBrokerId request handler: decodes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestProcessor
participant ControllerManager
participant Controller
Client->>RequestProcessor: RemotingCommand (ControllerGetNextBrokerId)
RequestProcessor->>RequestProcessor: decode GetNextBrokerIdRequestHeader
alt invalid header
RequestProcessor-->>Client: ControllerInvalidRequest
else valid
RequestProcessor->>ControllerManager: controller()
ControllerManager->>Controller: get_next_broker_id(header)
Controller-->>ControllerManager: RemotingCommand (response)
ControllerManager-->>RequestProcessor: response
RequestProcessor-->>Client: RemotingCommand (response)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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-robot
added
Difficulty level/Hard
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
|
🔊@aditya-rajpurohit 🚀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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rocketmq-controller/src/processor/controller_request_processor.rs (1)
595-606: Success logging doesn't verify actual allocation result.The current logging assumes any
Some(response)is a success, but the response could contain an error code. Compare withhandle_apply_broker_id(lines 710-725) which checksresponse.code() == ResponseCode::Successbefore logging success.♻️ Suggested improvement for accurate logging
// Log result if let Some(res) = &response { - info!( - "Allocated broker_id response created for cluster={}, broker={}", - request_header.cluster_name, request_header.broker_name - ); + if res.code() == ResponseCode::Success as i32 { + info!( + "GetNextBrokerId succeeded for cluster={}, broker={}", + request_header.cluster_name, request_header.broker_name + ); + } else { + warn!( + "GetNextBrokerId failed: cluster={}, broker={}, code={}, remark={:?}", + request_header.cluster_name, + request_header.broker_name, + res.code(), + res.remark() + ); + } } else { warn!( "Failed to allocate broker_id for cluster={}, broker={}",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-controller/src/processor/controller_request_processor.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: WaterWhisperer
Repo: mxsm/rocketmq-rust PR: 5582
File: rocketmq-controller/src/controller/open_raft_controller.rs:182-247
Timestamp: 2026-01-09T14:29:58.048Z
Learning: In the ApplyBrokerId operation across both OpenRaftController and RaftRsController implementations, broker_addr is intentionally set to String::new() (empty string) when constructing ControllerRequest::ApplyBrokerId. This matches the design of the Java RocketMQ reference implementation. The broker address is populated through other operations (e.g., RegisterBroker), as ApplyBrokerId focuses solely on broker ID allocation/reservation.
📚 Learning: 2026-01-09T14:29:58.048Z
Learnt from: WaterWhisperer
Repo: mxsm/rocketmq-rust PR: 5582
File: rocketmq-controller/src/controller/open_raft_controller.rs:182-247
Timestamp: 2026-01-09T14:29:58.048Z
Learning: In the ApplyBrokerId operation across both OpenRaftController and RaftRsController implementations, broker_addr is intentionally set to String::new() (empty string) when constructing ControllerRequest::ApplyBrokerId. This matches the design of the Java RocketMQ reference implementation. The broker address is populated through other operations (e.g., RegisterBroker), as ApplyBrokerId focuses solely on broker ID allocation/reservation.
Applied to files:
rocketmq-controller/src/processor/controller_request_processor.rs
📚 Learning: 2025-12-28T07:21:24.117Z
Learnt from: WaterWhisperer
Repo: mxsm/rocketmq-rust PR: 5099
File: rocketmq-remoting/src/protocol/header/get_producer_connection_list_request_header.rs:29-32
Timestamp: 2025-12-28T07:21:24.117Z
Learning: In the rocketmq-remoting/src/protocol/header/ module, header structs consistently use public fields rather than private fields with getters/setters. This is an established pattern across the codebase.
Applied to files:
rocketmq-controller/src/processor/controller_request_processor.rs
🧬 Code graph analysis (1)
rocketmq-controller/src/processor/controller_request_processor.rs (4)
rocketmq-controller/src/controller/controller_manager.rs (1)
controller(620-622)rocketmq-error/src/unified.rs (1)
request_header_error(471-473)rocketmq-controller/src/heartbeat/broker_live_info.rs (1)
broker_name(72-74)rocketmq-remoting/src/protocol/remoting_command.rs (1)
create_response_command_with_code_remark(187-192)
⏰ 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). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
🔇 Additional comments (3)
rocketmq-controller/src/processor/controller_request_processor.rs (3)
99-99: LGTM!The import follows the established pattern for controller request headers in this file.
570-586: LGTM!The validation logic for
cluster_nameandbroker_nameis correct and consistent with the pattern used inhandle_apply_broker_id. The appropriateControllerInvalidRequestresponse code is returned with clear error messages.
557-563: LGTM!The header decoding follows the established pattern with proper error handling and logging. The
request_header_errorhelper is used consistently with other handlers.
aditya-rajpurohit
force-pushed
the
feature-5540
branch
from
e569323 to
0088f0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rocketmq-controller/src/processor/controller_request_processor.rs (1)
596-606: Response logging doesn't distinguish success from error responses.The current logging assumes
Some(response)means success, but the response could contain an error code (e.g.,ControllerBrokerIdInvalid). Compare withhandle_apply_broker_id(lines 710-736) which properly inspectsresponse.code().♻️ Suggested improvement for accurate logging
// Log result - if let Some(_res) = &response { - info!( - "Allocated broker_id response created for cluster={}, broker={}", - request_header.cluster_name, request_header.broker_name - ); - } else { - warn!( - "Failed to allocate broker_id for cluster={}, broker={}", - request_header.cluster_name, request_header.broker_name - ); - } + match &response { + Some(res) if res.code() == ResponseCode::Success as i32 => { + info!( + "GetNextBrokerId succeeded: cluster={}, broker={}", + request_header.cluster_name, request_header.broker_name + ); + } + Some(res) => { + warn!( + "GetNextBrokerId failed: cluster={}, broker={}, code={}, remark={:?}", + request_header.cluster_name, + request_header.broker_name, + res.code(), + res.remark() + ); + } + None => { + warn!( + "GetNextBrokerId returned no response for cluster={}, broker={}", + request_header.cluster_name, request_header.broker_name + ); + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-controller/src/processor/controller_request_processor.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: WaterWhisperer
Repo: mxsm/rocketmq-rust PR: 5582
File: rocketmq-controller/src/controller/open_raft_controller.rs:182-247
Timestamp: 2026-01-09T14:29:58.048Z
Learning: In the ApplyBrokerId operation across both OpenRaftController and RaftRsController implementations, broker_addr is intentionally set to String::new() (empty string) when constructing ControllerRequest::ApplyBrokerId. This matches the design of the Java RocketMQ reference implementation. The broker address is populated through other operations (e.g., RegisterBroker), as ApplyBrokerId focuses solely on broker ID allocation/reservation.
📚 Learning: 2026-01-09T14:29:58.048Z
Learnt from: WaterWhisperer
Repo: mxsm/rocketmq-rust PR: 5582
File: rocketmq-controller/src/controller/open_raft_controller.rs:182-247
Timestamp: 2026-01-09T14:29:58.048Z
Learning: In the ApplyBrokerId operation across both OpenRaftController and RaftRsController implementations, broker_addr is intentionally set to String::new() (empty string) when constructing ControllerRequest::ApplyBrokerId. This matches the design of the Java RocketMQ reference implementation. The broker address is populated through other operations (e.g., RegisterBroker), as ApplyBrokerId focuses solely on broker ID allocation/reservation.
Applied to files:
rocketmq-controller/src/processor/controller_request_processor.rs
📚 Learning: 2025-12-28T07:21:24.117Z
Learnt from: WaterWhisperer
Repo: mxsm/rocketmq-rust PR: 5099
File: rocketmq-remoting/src/protocol/header/get_producer_connection_list_request_header.rs:29-32
Timestamp: 2025-12-28T07:21:24.117Z
Learning: In the rocketmq-remoting/src/protocol/header/ module, header structs consistently use public fields rather than private fields with getters/setters. This is an established pattern across the codebase.
Applied to files:
rocketmq-controller/src/processor/controller_request_processor.rs
🧬 Code graph analysis (1)
rocketmq-controller/src/processor/controller_request_processor.rs (4)
rocketmq-controller/src/controller/controller_manager.rs (1)
controller(620-622)rocketmq-error/src/unified.rs (1)
request_header_error(471-473)rocketmq-controller/src/heartbeat/broker_live_info.rs (1)
broker_name(72-74)rocketmq-remoting/src/protocol/remoting_command.rs (1)
create_response_command_with_code_remark(187-192)
⏰ 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). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
🔇 Additional comments (2)
rocketmq-controller/src/processor/controller_request_processor.rs (2)
99-99: LGTM!Import follows the existing pattern for controller request headers and is correctly placed.
557-593: Implementation correctly follows established patterns.The handler properly:
- Decodes the request header with appropriate error handling
- Validates both
cluster_nameandbroker_namebefore forwarding- Delegates allocation to the controller via Raft consensus
Minor nit: The
.to_string()calls on lines 575 and 584 are unnecessary sincecreate_response_command_with_code_remarkacceptsimpl Into<CheetahString>, but this is cosmetic.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5683 +/- ##
=======================================
Coverage 39.15% 39.15%
=======================================
Files 820 820
Lines 113010 113040 +30
=======================================
+ Hits 44246 44265 +19
- Misses 68764 68775 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
4 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #5540
Brief Description
This PR implements the
handle_get_next_broker_idmethod inControllerRequestProcessorto handle broker ID allocation requests. This method:GetNextBrokerIdRequestHeaderfrom the incoming request.cluster_nameandbroker_namefields.RemotingCommandresponse containing the allocated broker ID or an error if allocation fails.This ensures that new brokers joining the cluster are assigned unique IDs and supports cluster scaling and broker replacement.
How Did You Test This Change?
cargo buildcargo testSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.