Repository navigation
[ISSUE #3871]🚀Implement sync_all method in SlaveSynchronize for comprehensive synchronization - #3872
Merged
Merged
Conversation
…ehensive synchronization
Contributor
WalkthroughAdds orchestration logic to sync_all in rocketmq-broker/src/slave/slave_synchronize.rs to invoke several new private async sync helpers and a conditional timer metrics sync. Helper methods are stubs that log “not implemented”. sync_timer_check_point remains unimplemented. Public API surface is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor BrokerSlave as Slave Node
participant SlaveSync as SlaveSynchronize
participant StoreCfg as MessageStore.Config
note over BrokerSlave,SlaveSync: High-level orchestration of sync_all
BrokerSlave->>SlaveSync: sync_all()
activate SlaveSync
SlaveSync->>SlaveSync: sync_topic_config()
SlaveSync->>SlaveSync: sync_consumer_offset()
SlaveSync->>SlaveSync: sync_delay_offset()
SlaveSync->>SlaveSync: sync_subscription_group_config()
SlaveSync->>SlaveSync: sync_message_request_mode()
SlaveSync->>StoreCfg: read timer_wheel_enable
alt timer_wheel_enable == true
SlaveSync->>SlaveSync: sync_timer_metrics()
else
note over SlaveSync: Skip timer metrics sync
end
deactivate SlaveSync
SlaveSync-->>BrokerSlave: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
August 28, 2025 15:11
Collaborator
|
🔊@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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3872 +/- ##
==========================================
- Coverage 27.01% 27.00% -0.01%
==========================================
Files 556 556
Lines 78923 78952 +29
==========================================
Hits 21318 21318
- Misses 57605 57634 +29 ☔ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
rocketmq-broker/src/slave/slave_synchronize.rs(1 hunks)
⏰ 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: build (windows-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: auto-approve
- GitHub Check: build
- GitHub Check: test
Comment on lines
+60
to
73
| self.sync_topic_config().await; | ||
| self.sync_consumer_offset().await; | ||
| self.sync_delay_offset().await; | ||
| self.sync_subscription_group_config().await; | ||
| self.sync_message_request_mode().await; | ||
| if self | ||
| .broker_runtime_inner | ||
| .message_store_unchecked() | ||
| .get_message_store_config() | ||
| .timer_wheel_enable | ||
| { | ||
| self.sync_timer_metrics().await; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard when master is unset; add start/end logs; include timer checkpoint under the same gate
Avoid doing a “comprehensive sync” without a master address, and log lifecycle. Also include sync_timer_check_point() alongside metrics when the timer wheel is enabled.
Apply this diff within the shown range:
- self.sync_topic_config().await;
- self.sync_consumer_offset().await;
- self.sync_delay_offset().await;
- self.sync_subscription_group_config().await;
- self.sync_message_request_mode().await;
- if self
- .broker_runtime_inner
- .message_store_unchecked()
- .get_message_store_config()
- .timer_wheel_enable
- {
- self.sync_timer_metrics().await;
- }
+ info!("Start sync_all");
+ if self.master_addr.is_none() {
+ warn!("Skip sync_all: master_addr is not set");
+ return;
+ }
+ self.sync_topic_config().await;
+ self.sync_consumer_offset().await;
+ self.sync_delay_offset().await;
+ self.sync_subscription_group_config().await;
+ self.sync_message_request_mode().await;
+ if self
+ .broker_runtime_inner
+ .message_store_unchecked()
+ .get_message_store_config()
+ .timer_wheel_enable
+ {
+ self.sync_timer_metrics().await;
+ self.sync_timer_check_point().await;
+ }
+ info!("Completed sync_all");Add this import outside the range:
use tracing::warn;Optional: run the independent syncs concurrently to reduce wall time:
- self.sync_topic_config().await;
- self.sync_consumer_offset().await;
- self.sync_delay_offset().await;
- self.sync_subscription_group_config().await;
- self.sync_message_request_mode().await;
+ tokio::join!(
+ self.sync_topic_config(),
+ self.sync_consumer_offset(),
+ self.sync_delay_offset(),
+ self.sync_subscription_group_config(),
+ self.sync_message_request_mode(),
+ );
@@
- self.sync_timer_metrics().await;
- self.sync_timer_check_point().await;
+ tokio::join!(self.sync_timer_metrics(), self.sync_timer_check_point());
Comment on lines
+79
to
+101
| async fn sync_topic_config(&self) { | ||
| error!("SlaveSynchronize::syncTopicConfig is not implemented yet"); | ||
| } | ||
|
|
||
| async fn sync_consumer_offset(&self) { | ||
| error!("SlaveSynchronize::syncTopicConfig is not implemented yet"); | ||
| } | ||
|
|
||
| async fn sync_delay_offset(&self) { | ||
| error!("SlaveSynchronize::syncTopicConfig is not implemented yet"); | ||
| } | ||
|
|
||
| async fn sync_subscription_group_config(&self) { | ||
| error!("SlaveSynchronize::syncTopicConfig is not implemented yet"); | ||
| } | ||
|
|
||
| async fn sync_message_request_mode(&self) { | ||
| error!("SlaveSynchronize::syncTopicConfig is not implemented yet"); | ||
| } | ||
|
|
||
| async fn sync_timer_metrics(&self) { | ||
| error!("SlaveSynchronize::syncTopicConfig is not implemented yet"); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix copy/paste in log messages and lower severity from error to warn
All stubs log “syncTopicConfig” at error level, which is misleading and noisy. Use the correct function name and a warning level.
Apply this diff:
async fn sync_topic_config(&self) {
- error!("SlaveSynchronize::syncTopicConfig is not implemented yet");
+ warn!("SlaveSynchronize::sync_topic_config is not implemented yet");
}
async fn sync_consumer_offset(&self) {
- error!("SlaveSynchronize::syncTopicConfig is not implemented yet");
+ warn!("SlaveSynchronize::sync_consumer_offset is not implemented yet");
}
async fn sync_delay_offset(&self) {
- error!("SlaveSynchronize::syncTopicConfig is not implemented yet");
+ warn!("SlaveSynchronize::sync_delay_offset is not implemented yet");
}
async fn sync_subscription_group_config(&self) {
- error!("SlaveSynchronize::syncTopicConfig is not implemented yet");
+ warn!("SlaveSynchronize::sync_subscription_group_config is not implemented yet");
}
async fn sync_message_request_mode(&self) {
- error!("SlaveSynchronize::syncTopicConfig is not implemented yet");
+ warn!("SlaveSynchronize::sync_message_request_mode is not implemented yet");
}
async fn sync_timer_metrics(&self) {
- error!("SlaveSynchronize::syncTopicConfig is not implemented yet");
+ warn!("SlaveSynchronize::sync_timer_metrics is not implemented yet");
}Add this import (outside range) if not present:
use tracing::warn;🤖 Prompt for AI Agents
In rocketmq-broker/src/slave/slave_synchronize.rs around lines 79 to 101, each
async stub currently logs the wrong message ("syncTopicConfig") at error level;
update each function to call warn! with the correct function-specific message
(e.g., "SlaveSynchronize::sync_topic_config is not implemented yet",
"SlaveSynchronize::sync_consumer_offset is not implemented yet", etc.), and add
the import use tracing::warn; near the other imports if it is not already
present.
rocketmq-rust-bot
added
approved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Issue(s) This PR Fixes(Closes)
Fixes #3871
Brief Description
How Did You Test This Change?
Summary by CodeRabbit