[ISSUE #2242]🚀Optimize BrokerRuntime shutdown💥 #2267
Conversation
WalkthroughThe pull request focuses on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
rocketmq-rust-robot
added
feature🚀
|
🔊@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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2267 +/- ##
==========================================
- Coverage 28.29% 28.25% -0.04%
==========================================
Files 504 504
Lines 72233 72320 +87
==========================================
Hits 20437 20437
- Misses 51796 51883 +87 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rocketmq-broker/src/broker_runtime.rs (2)
189-193: Simplify the type signature forconsumer_ids_change_listenerThe current type signature uses unnecessary nesting of
BoxinsideArc. SinceArccan hold trait objects and provides shared ownership, you can simplify the type by removing theBox. This enhances readability and reduces complexity.Apply this diff to simplify the type:
-let consumer_ids_change_listener: Arc< - Box<dyn ConsumerIdsChangeListener + Send + Sync + 'static>, -> = Arc::new(Box::new(DefaultConsumerIdsChangeListener {})); +let consumer_ids_change_listener: Arc<dyn ConsumerIdsChangeListener + Send + Sync + 'static> = Arc::new(DefaultConsumerIdsChangeListener {});
265-265: Consolidate initialization oftopic_queue_mapping_clean_serviceThe
topic_queue_mapping_clean_serviceis initialized in bothBrokerRuntime::new()andinitialize_resources(). This may lead to unintended overwrites or inconsistencies. Consider initializing it only in one place to ensure consistent behavior.Apply this diff to remove redundant initialization:
// In BrokerRuntime::new() self.inner.topic_queue_mapping_clean_service = Some(TopicQueueMappingCleanService);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-broker/src/broker_runtime.rs(13 hunks)rocketmq-broker/src/client/default_consumer_ids_change_listener.rs(2 hunks)rocketmq-broker/src/schedule/schedule_message_service.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: test
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-broker/src/broker_runtime.rs (1)
282-285: Ensurebroker_runtimeis properly shut downThe added code correctly shuts down the
broker_runtimeif it exists, ensuring that all associated resources are released.
| if let Some(notification_processor) = self.inner.notification_processor.as_mut() { | ||
| notification_processor.shutdown(); | ||
| } | ||
| self.consumer_ids_change_listener.shutdown(); |
There was a problem hiding this comment.
Implement the shutdown method in consumer_ids_change_listener
The shutdown method of DefaultConsumerIdsChangeListener currently only logs a warning. This means any resources or background tasks it handles may not be properly released or stopped. Please implement the necessary shutdown logic to ensure clean resource management.
| @@ -26,6 +28,6 @@ | |||
| fn handle(&self, _event: ConsumerGroupEvent, _group: &str, _args: &[&dyn Any]) {} | |||
|
|
|||
| fn shutdown(&self) { | |||
| todo!() | |||
| warn!("DefaultConsumerIdsChangeListener shutdown not implemented"); | |||
There was a problem hiding this comment.
Implement the shutdown method properly
Currently, the shutdown method logs a warning indicating it is not implemented. This can lead to improper resource cleanup during shutdown. Please provide an appropriate implementation to ensure resources are released and any background tasks are properly terminated.
| warn!("ScheduleMessageService encode_pretty not implemented"); | ||
| "".to_string() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide implementation for encode_pretty method
The encode_pretty method logs a warning and returns an empty string, which may cause issues if other components expect a proper configuration string. Implement this method to return the serialized configuration data in a pretty-printed format.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #2242
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores