[ISSUE #1076]PullMessageService and RebalanceService add shutdown method #1077
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
|
🚀Thanks for your contribution🎉. CodeRabbit(AI) will review your code 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: 3
🧹 Outside diff range and nitpick comments (4)
rocketmq-client/src/consumer/consumer_impl/re_balance/rebalance_service.rs (1)
103-103: Correct log message to reflectRebalanceServiceThe log message mentions
pull_tx, but this is in the context ofRebalanceService. Update the log message to accurately reflect the component.Apply this diff:
if let Err(e) = self.tx_shutdown.as_ref().unwrap().send(()) { - warn!("Failed to send shutdown signal to pull_tx, error: {:?}", e); + warn!("Failed to send shutdown signal to RebalanceService, error: {:?}", e); }rocketmq-client/src/consumer/consumer_impl/pull_message_service.rs (3)
Line range hint
49-59: Remove commented-out code for clarityThe old
whileloop for processing requests is commented out (lines 49-59). Keeping dead code can clutter the codebase and make maintenance harder. Since this code is replaced by the newtokio::select!loop, consider removing the commented-out code.Apply this diff to clean up the code:
- /*while let Some(request) = rx.recv().await { - if request.get_message_request_mode() == MessageRequestMode::Pull { - let pull_request = - unsafe { *Box::from_raw(Box::into_raw(request) as *mut PullRequest) }; - PullMessageService::pull_message(pull_request, instance.as_mut()).await; - } else { - let pop_request = - unsafe { *Box::from_raw(Box::into_raw(request) as *mut PopRequest) }; - PullMessageService::pop_message(pop_request, instance.as_mut()).await; - } - - }*/
60-63: Redundant shutdown check before entering the loopThe check
if shutdown.is_shutdown()before entering the loop (lines 60-63) might be unnecessary since thetokio::select!inside the loop handles the shutdown signal. This initial check could be removed to simplify the code.Apply this diff to remove the redundant check:
- if shutdown.is_shutdown() { - info!("PullMessageService shutdown"); - return; - }
67-68: Consolidate shutdown log messages to avoid duplicationThe message
"PullMessageService shutdown"is logged multiple times when shutting down (lines 67-68 and 82-83). This can lead to repetitive log entries. Consider logging the shutdown message only once to keep the logs clean.Apply this diff to log the shutdown message once:
_ = shutdown.recv() => { - info!("PullMessageService shutdown"); } } - if shutdown.is_shutdown() { - info!("PullMessageService shutdown"); - break; - } + break; } + info!("PullMessageService shutdown");Also applies to: 82-83
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- rocketmq-client/src/consumer/consumer_impl/pull_message_service.rs (4 hunks)
- rocketmq-client/src/consumer/consumer_impl/re_balance/rebalance_service.rs (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
rocketmq-client/src/consumer/consumer_impl/re_balance/rebalance_service.rs (1)
Line range hint
65-80: Shutdown handling inRebalanceServiceis correctly implementedThe implementation of the shutdown mechanism in
RebalanceServiceusingtokio::select!and theShutdownstruct allows for graceful and responsive shutdown of the service.
rocketmq-client/src/consumer/consumer_impl/re_balance/rebalance_service.rs
Outdated
Show resolved
Hide resolved
| if shutdown.is_shutdown() { | ||
| info!("PullMessageService shutdown"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify shutdown handling after the loop
After the tokio::select! loop, there's a check for shutdown.is_shutdown() (lines 81-84). Since the loop exits when a shutdown signal is received, this check and the associated break statement may be unnecessary. Consider restructuring to eliminate redundant code.
Apply this diff to simplify the loop:
}
- if shutdown.is_shutdown() {
- info!("PullMessageService shutdown");
- break;
- }
}📝 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.
| if shutdown.is_shutdown() { | |
| info!("PullMessageService shutdown"); | |
| break; | |
| } | |
| if shutdown.is_shutdown() { | |
| info!("PullMessageService shutdown"); | |
| break; | |
| } |
rocketmq-client/src/consumer/consumer_impl/pull_message_service.rs
Outdated
Show resolved
Hide resolved
…e_service.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…e.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
🚀Thanks for your contribution🎉. CodeRabbit(AI) will review your code first🔥 |
1 similar comment
|
🚀Thanks for your contribution🎉. CodeRabbit(AI) will review your code first🔥 |
|
🚀Thanks for your contribution🎉. CodeRabbit(AI) will review your code first🔥 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1077 +/- ##
==========================================
- Coverage 19.89% 19.88% -0.02%
==========================================
Files 427 427
Lines 35702 35720 +18
==========================================
Hits 7104 7104
- Misses 28598 28616 +18 ☔ View full report in Codecov by Sentry. |
Which Issue(s) This PR Fixes(Closes)
Fixes #1076
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
shutdownmethod to gracefully handle shutdown requests.Bug Fixes