Conversation
|
🔊@oopscompiled 🚀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
WalkthroughThis PR consolidates and simplifies three components: error handling in broker runtime changes from unsafe unwrap to explicit panic, query message processor constructor is simplified by removing unused parameters, and the service task trait hierarchy is consolidated by merging ServiceTaskInner into ServiceTask with updated method signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 5❌ Failed checks (2 warnings, 3 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request addresses three enhancement issues aimed at code cleanup: removing commented-out dead code from service_task.rs and query_message_processor.rs, and replacing an unsafe unwrap_unchecked() call with unimplemented!() in broker_runtime.rs.
Changes:
- Removed commented-out
ServiceTaskInnertrait definition from service_task.rs - Cleaned up commented parameters and fields in QueryMessageProcessor constructor
- Replaced unsafe
unwrap_unchecked()withunimplemented!()fortimer_message_store_uncheckedmethod
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rocketmq/src/task/service_task.rs | Removes unused commented-out trait definition |
| rocketmq-broker/src/processor/query_message_processor.rs | Cleans up commented parameters in constructor |
| rocketmq-broker/src/broker_runtime.rs | Replaces unsafe unwrap with unimplemented macro |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[inline] | ||
| pub fn timer_message_store_unchecked(&self) -> &TimerMessageStore { | ||
| unsafe { self.timer_message_store.as_ref().unwrap_unchecked() } | ||
| unimplemented!("timer_message_store_unchecked is not implemented yet") |
There was a problem hiding this comment.
This change will cause a runtime panic when timer messages are processed. The method timer_message_store_unchecked() is actively called in rocketmq-broker/src/util/hook_utils.rs:157 within the handle_schedule_message function, which is used by ScheduleMessageHook for processing messages with timer properties. When a timer message is handled (and timer_wheel_enable is true), this will panic instead of returning the timer message store. The original unsafe unwrap_unchecked() should be replaced with a safe alternative like unwrap() or proper error handling, not unimplemented!().
| unimplemented!("timer_message_store_unchecked is not implemented yet") | |
| self.timer_message_store.as_ref().unwrap() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rocketmq-broker/src/broker_runtime.rs`:
- Around line 1757-1759: timer_message_store_unchecked currently calls
unimplemented!() which will panic when handle_schedule_message invokes it (when
timer_wheel_enable is true); replace the panic with a real implementation or a
safe API: either return a reference to the existing TimerMessageStore instance
(implement the body of timer_message_store_unchecked to retrieve and return the
stored TimerMessageStore), or change the public API to return
Result<&TimerMessageStore, Error> and propagate errors up from
handle_schedule_message, or gate the method behind a feature flag so callers
cannot call it when timer wheel is disabled; locate
timer_message_store_unchecked and the TimerMessageStore storage/field in the
broker_runtime impl and implement safe retrieval or alter its signature and
update handle_schedule_message accordingly.
| pub fn timer_message_store_unchecked(&self) -> &TimerMessageStore { | ||
| unsafe { self.timer_message_store.as_ref().unwrap_unchecked() } | ||
| unimplemented!("timer_message_store_unchecked is not implemented yet") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any callers of timer_message_store_unchecked across all Rust files
rg -n 'timer_message_store_unchecked' --type rustRepository: mxsm/rocketmq-rust
Length of output: 415
🏁 Script executed:
#!/bin/bash
# Get context around the caller at hook_utils.rs:157
sed -n '145,170p' rocketmq-broker/src/util/hook_utils.rs | cat -nRepository: mxsm/rocketmq-rust
Length of output: 1568
Do not use unimplemented!() — this method has a caller that will panic at runtime.
timer_message_store_unchecked() is called from rocketmq-broker/src/util/hook_utils.rs:157 in the handle_schedule_message() function, which is the core message scheduling handler. When timer_wheel_enable is true, any incoming timer message will trigger the unimplemented!() panic.
Either implement the method properly or remove the public API. If the feature is not ready, consider returning an error or wrapping the method with a feature flag.
🤖 Prompt for AI Agents
In `@rocketmq-broker/src/broker_runtime.rs` around lines 1757 - 1759,
timer_message_store_unchecked currently calls unimplemented!() which will panic
when handle_schedule_message invokes it (when timer_wheel_enable is true);
replace the panic with a real implementation or a safe API: either return a
reference to the existing TimerMessageStore instance (implement the body of
timer_message_store_unchecked to retrieve and return the stored
TimerMessageStore), or change the public API to return
Result<&TimerMessageStore, Error> and propagate errors up from
handle_schedule_message, or gate the method behind a feature flag so callers
cannot call it when timer wheel is disabled; locate
timer_message_store_unchecked and the TimerMessageStore storage/field in the
broker_runtime impl and implement safe retrieval or alter its signature and
update handle_schedule_message accordingly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6353 +/- ##
==========================================
- Coverage 42.62% 42.46% -0.17%
==========================================
Files 912 919 +7
Lines 128056 129254 +1198
==========================================
+ Hits 54590 54893 +303
- Misses 73466 74361 +895 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Release Notes
Refactor