[ISSUE #4075]🚀Implement Broker on_master_offline #4076
Conversation
WalkthroughIntroduces reference-based master address propagation in slave synchronization, adds accessor methods to BrokerRuntimeInner, modifies update_slave_master_addr to use references, and implements/updates on_master_offline to clear and propagate master address changes to HA via message_store. Adjusts SlaveSynchronize::set_master_addr to accept Option<&CheetahString> and clone internally. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BR as BrokerRuntimeInner
participant SS as SlaveSynchronize
participant MS as MessageStore (HA)
rect rgba(220,240,255,0.4)
note over BR: on_master_offline
BR->>SS: access via slave_synchronize_mut(_)
alt master_addr is non-empty
BR->>SS: set_master_addr(None)
BR->>MS: update HA master addr (None)
MS-->>BR: ack
else master_addr empty
BR-->>BR: no-op
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)rocketmq-broker/src/broker_runtime.rs (1)
⏰ 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)
🔇 Additional comments (5)
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 |
|
🔊@mxsm 🚀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
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
There was a problem hiding this comment.
Pull Request Overview
Implements broker behavior for handling master offline events and adjusts how the slave tracks the master address.
- Change SlaveSynchronize::set_master_addr to accept a borrowed address and update call sites
- Add unchecked accessors for SlaveSynchronize in BrokerRuntimeInner and implement on_master_offline to clear master state and update HA address
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rocketmq-broker/src/slave/slave_synchronize.rs | Changes set_master_addr to take Option<&CheetahString> and clone internally |
| rocketmq-broker/src/broker_runtime.rs | Updates call sites for set_master_addr, adds unchecked accessors, and implements on_master_offline logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let slave_synchronize = self.slave_synchronize_unchecked(); | ||
| if let Some(master_addr) = slave_synchronize.master_addr() { | ||
| if !master_addr.is_empty() { | ||
| //close channels | ||
| } | ||
| } | ||
| self.slave_synchronize_mut_unchecked().set_master_addr(None); |
There was a problem hiding this comment.
Using unwrap_unchecked via slave_synchronize_unchecked()/slave_synchronize_mut_unchecked() here is UB if self.slave_synchronize is None. Replace with a safe path that gracefully no-ops when absent.
| let slave_synchronize = self.slave_synchronize_unchecked(); | |
| if let Some(master_addr) = slave_synchronize.master_addr() { | |
| if !master_addr.is_empty() { | |
| //close channels | |
| } | |
| } | |
| self.slave_synchronize_mut_unchecked().set_master_addr(None); | |
| if let Some(slave_synchronize) = self.slave_synchronize() { | |
| if let Some(master_addr) = slave_synchronize.master_addr() { | |
| if !master_addr.is_empty() { | |
| //close channels | |
| } | |
| } | |
| } | |
| if let Some(slave_synchronize_mut) = self.slave_synchronize_mut() { | |
| slave_synchronize_mut.set_master_addr(None); | |
| } |
Copilot uses AI. Check for mistakes.
| pub fn update_slave_master_addr(&mut self, master_addr: Option<CheetahString>) { | ||
| if let Some(ref mut slave) = self.slave_synchronize { | ||
| slave.set_master_addr(master_addr); | ||
| slave.set_master_addr(master_addr.as_ref()); | ||
| }; |
There was a problem hiding this comment.
This clones master_addr inside set_master_addr even though an owned Option is already available, causing an avoidable allocation. Either (a) change set_master_addr to take Option by value and pass master_addr directly, or (b) change update_slave_master_addr to accept Option<&CheetahString> to avoid constructing an owned value.
Copilot uses AI. Check for mistakes.
| pub fn set_master_addr(&mut self, addr: Option<&CheetahString>) { | ||
| let addr = addr.cloned(); | ||
| if self.master_addr == addr { | ||
| return; |
There was a problem hiding this comment.
[nitpick] Changing the signature from Option<impl Into> to Option<&CheetahString> reduces ergonomics and forces an internal clone, losing the zero-cost move path for owned values. Consider restoring the owned-taking API (e.g., pub fn set_master_addr(&mut self, addr: Option)) and, if needed, adding a separate convenience for borrowed inputs that clones explicitly.
Copilot uses AI. Check for mistakes.
| #[inline] | ||
| pub fn slave_synchronize_unchecked(&self) -> &SlaveSynchronize<MS> { | ||
| unsafe { self.slave_synchronize.as_ref().unwrap_unchecked() } | ||
| } |
There was a problem hiding this comment.
These unchecked accessors rely on invariants about self.slave_synchronize being Some but provide no Safety docs specifying the required preconditions. Please add doc-comments with a Safety section detailing the invariant and when these may be called, or keep them private and prefer the checked variants at call sites.
Copilot uses AI. Check for mistakes.
| #[inline] | ||
| pub fn slave_synchronize_mut_unchecked(&mut self) -> &mut SlaveSynchronize<MS> { | ||
| unsafe { self.slave_synchronize.as_mut().unwrap_unchecked() } | ||
| } |
There was a problem hiding this comment.
These unchecked accessors rely on invariants about self.slave_synchronize being Some but provide no Safety docs specifying the required preconditions. Please add doc-comments with a Safety section detailing the invariant and when these may be called, or keep them private and prefer the checked variants at call sites.
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4076 +/- ##
==========================================
- Coverage 26.49% 26.48% -0.01%
==========================================
Files 574 574
Lines 81182 81200 +18
==========================================
Hits 21507 21507
- Misses 59675 59693 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
3 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4075
Brief Description
How Did You Test This Change?
Summary by CodeRabbit