Repository navigation
[ISSUE #3843]Add test coverage for multi_dispatch_utils in queue.rs (LMQ key, dispatch checks) - #3844
Conversation
…LMQ key, dispatch checks)
WalkthroughIntroduces a new internal module multi_dispatch_utils in rocketmq-store/src/queue.rs with functions to determine multi-dispatch behavior and compute LMQ queue keys, and adds unit tests covering key formatting, gating conditions, and property validation for dispatch requests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Producer
participant MessageStore
participant Queue as multi_dispatch_utils
participant Dispatcher
Producer->>MessageStore: Append message(topic, properties)
MessageStore->>Queue: is_need_handle_multi_dispatch(cfg, topic)
alt Multi-dispatch enabled for topic
MessageStore->>Queue: check_multi_dispatch_queue(cfg, dispatchRequest)
alt Properties valid
Note right of Queue: mdq & mqo present and non-empty
Queue-->>MessageStore: true
MessageStore->>Dispatcher: Dispatch to multiple queues
Dispatcher-->>MessageStore: Ack
else Properties invalid
Queue-->>MessageStore: false
MessageStore->>Dispatcher: Dispatch to default queue
end
else Not needed
MessageStore->>Dispatcher: Dispatch to default queue
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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
|
|
🔊@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 #3844 +/- ##
==========================================
+ Coverage 26.91% 27.03% +0.11%
==========================================
Files 556 556
Lines 78763 78858 +95
==========================================
+ Hits 21201 21318 +117
+ Misses 57562 57540 -22 ☔ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
rocketmq-store/src/queue.rs (2)
132-162: Optional: strengthen gating coverage with negative cases insidecheck_multi_dispatch_queue.You already test
is_need_handle_multi_dispatchseparately. Consider adding a quick negative case incheck_multi_dispatch_queuewhere the topic is a retry/system/schedule topic so that early-return gating is covered within this path too.If you want, I can draft a small insertion covering a
%RETRY%topic with valid props and assertingcheck_multi_dispatch_queuereturnsfalse.
100-115: Optional: early-return style improves readability.Not required, but you can keep the logic flatter and a bit more idiomatic with early returns and a helper predicate.
- pub fn check_multi_dispatch_queue(cfg: &MessageStoreConfig, dr: &DispatchRequest) -> bool { - if !is_need_handle_multi_dispatch(cfg, dr.topic.as_str()) { - return false; - } - let Some(props) = &dr.properties_map else { - return false; - }; - if props.is_empty() { - return false; - } - - let mdq = props.get(MessageConst::PROPERTY_INNER_MULTI_DISPATCH); - let mqo = props.get(MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET); - - matches!((mdq, mqo), (Some(a), Some(b)) if !a.is_empty() && !b.is_empty()) - } + pub fn check_multi_dispatch_queue(cfg: &MessageStoreConfig, dr: &DispatchRequest) -> bool { + if !is_need_handle_multi_dispatch(cfg, dr.topic.as_str()) { + return false; + } + let Some(props) = &dr.properties_map else { return false }; + if props.is_empty() { + return false; + } + let mdq = props.get(MessageConst::PROPERTY_INNER_MULTI_DISPATCH); + let mqo = props.get(MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET); + matches!( + (mdq, mqo), + (Some(a), Some(b)) + if !a.as_str().trim().is_empty() && !b.as_str().trim().is_empty() + ) + }
📜 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-store/src/queue.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-store/src/queue.rs (2)
rocketmq-store/src/config/message_store_config.rs (1)
default(818-1007)rocketmq-store/src/base/dispatch_request.rs (1)
default(48-70)
⏰ 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
- GitHub Check: test
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: auto-approve
🔇 Additional comments (5)
rocketmq-store/src/queue.rs (5)
89-91: LMQ key formatting is correct and minimal.
format!("{queue_name}-0")aligns with the requirement that "Q" → "Q-0". No change needed.
171-177: Good negative case for missing properties.This correctly verifies the early return when
properties_mapisNone.
178-184: Good negative case for empty properties.This ensures the map-presence check is not enough without keys.
185-200: Happy-path coverage looks good.Both required props present with non-empty values leads to
true— matches the intended logic.
127-162: Due to the sandbox’s lack of a mounted /proc, we’re unable to automatically execute the Rust test harness here. Please run the following locally to confirm that your changes haven’t broken any assertions:# From the repo root, verify only queue.rs tests cargo test -p rocketmq-store --lib queue::testsOnce you’ve confirmed that all tests pass, this comment can be resolved.
| pub fn is_need_handle_multi_dispatch(cfg: &MessageStoreConfig, topic: &str) -> bool { | ||
| cfg.enable_multi_dispatch | ||
| && !topic.starts_with(mix_all::RETRY_GROUP_TOPIC_PREFIX) | ||
| && !topic.starts_with(TopicValidator::SYSTEM_TOPIC_PREFIX) | ||
| && topic != TopicValidator::RMQ_SYS_SCHEDULE_TOPIC | ||
| } |
There was a problem hiding this comment.
System/retry/schedule topic gating logic looks right; fix the contradicting test expectation.
is_need_handle_multi_dispatch correctly excludes retry topics, system-topic-prefixed topics, and the schedule topic when enable_multi_dispatch is true. However, the test currently asserts true for a system-prefixed topic, which contradicts this logic and the stated objectives.
Apply this change in the tests to assert exclusion for system topics:
- assert!(multi_dispatch_utils::is_need_handle_multi_dispatch(
- &cfg,
- "RMQ_SYS_internal"
- ));
+ assert!(!multi_dispatch_utils::is_need_handle_multi_dispatch(
+ &cfg,
+ "RMQ_SYS_internal"
+ ));📝 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.
| pub fn is_need_handle_multi_dispatch(cfg: &MessageStoreConfig, topic: &str) -> bool { | |
| cfg.enable_multi_dispatch | |
| && !topic.starts_with(mix_all::RETRY_GROUP_TOPIC_PREFIX) | |
| && !topic.starts_with(TopicValidator::SYSTEM_TOPIC_PREFIX) | |
| && topic != TopicValidator::RMQ_SYS_SCHEDULE_TOPIC | |
| } | |
| - assert!(multi_dispatch_utils::is_need_handle_multi_dispatch( | |
| - &cfg, | |
| - "RMQ_SYS_internal" | |
| assert!(!multi_dispatch_utils::is_need_handle_multi_dispatch( | |
| &cfg, | |
| "RMQ_SYS_internal" | |
| )); |
🤖 Prompt for AI Agents
In rocketmq-store/src/queue.rs around lines 93 to 98, the function
is_need_handle_multi_dispatch correctly returns false for system-prefixed
topics, but the test expecting true for a system topic is contradictory; update
the test case to assert that is_need_handle_multi_dispatch returns false for a
topic starting with TopicValidator::SYSTEM_TOPIC_PREFIX (i.e., change the
expectation from true to false or replace the test input with a non-system
topic) so it matches the gating logic that excludes system topics when
enable_multi_dispatch is set.
| pub fn check_multi_dispatch_queue(cfg: &MessageStoreConfig, dr: &DispatchRequest) -> bool { | ||
| if !is_need_handle_multi_dispatch(cfg, dr.topic.as_str()) { | ||
| return false; | ||
| } | ||
| let Some(props) = &dr.properties_map else { | ||
| return false; | ||
| }; | ||
| if props.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| let mdq = props.get(MessageConst::PROPERTY_INNER_MULTI_DISPATCH); | ||
| let mqo = props.get(MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET); | ||
|
|
||
| matches!((mdq, mqo), (Some(a), Some(b)) if !a.is_empty() && !b.is_empty()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Treat whitespace-only property values as invalid (trim before emptiness check).
Per objectives, blank or empty property values should be invalid. Currently, " " passes the is_empty() check. Trim both values before testing emptiness.
let mdq = props.get(MessageConst::PROPERTY_INNER_MULTI_DISPATCH);
let mqo = props.get(MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET);
- matches!((mdq, mqo), (Some(a), Some(b)) if !a.is_empty() && !b.is_empty())
+ matches!(
+ (mdq, mqo),
+ (Some(a), Some(b))
+ if !a.as_str().trim().is_empty() && !b.as_str().trim().is_empty()
+ )This change aligns behavior with the intended validation and will also make the “blank value” test meaningful.
📝 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.
| pub fn check_multi_dispatch_queue(cfg: &MessageStoreConfig, dr: &DispatchRequest) -> bool { | |
| if !is_need_handle_multi_dispatch(cfg, dr.topic.as_str()) { | |
| return false; | |
| } | |
| let Some(props) = &dr.properties_map else { | |
| return false; | |
| }; | |
| if props.is_empty() { | |
| return false; | |
| } | |
| let mdq = props.get(MessageConst::PROPERTY_INNER_MULTI_DISPATCH); | |
| let mqo = props.get(MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET); | |
| matches!((mdq, mqo), (Some(a), Some(b)) if !a.is_empty() && !b.is_empty()) | |
| } | |
| pub fn check_multi_dispatch_queue(cfg: &MessageStoreConfig, dr: &DispatchRequest) -> bool { | |
| if !is_need_handle_multi_dispatch(cfg, dr.topic.as_str()) { | |
| return false; | |
| } | |
| let Some(props) = &dr.properties_map else { | |
| return false; | |
| }; | |
| if props.is_empty() { | |
| return false; | |
| } | |
| let mdq = props.get(MessageConst::PROPERTY_INNER_MULTI_DISPATCH); | |
| let mqo = props.get(MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET); | |
| matches!( | |
| (mdq, mqo), | |
| (Some(a), Some(b)) | |
| if !a.as_str().trim().is_empty() && !b.as_str().trim().is_empty() | |
| ) | |
| } |
🤖 Prompt for AI Agents
In rocketmq-store/src/queue.rs around lines 100 to 115, the check treats
whitespace-only property values as valid because it uses is_empty() directly;
change the emptiness test to trim both PROPERTY_INNER_MULTI_DISPATCH and
PROPERTY_INNER_MULTI_QUEUE_OFFSET values before checking emptiness (i.e., treat
values where value.trim().is_empty() as empty). Update the matches!/if condition
so it first obtains the Option<&String> for both properties and uses their
trimmed forms (or calls trim().is_empty()) to determine non-emptiness, returning
true only when both options are Some and both trimmed strings are non-empty.
| #[cfg(test)] | ||
| mod tests { | ||
| use rocketmq_common::common::message::MessageConst; | ||
| use rocketmq_common::common::topic::TopicValidator; | ||
|
|
||
| use super::*; | ||
| use crate::base::dispatch_request::DispatchRequest; | ||
| use crate::config::message_store_config::MessageStoreConfig; | ||
|
|
There was a problem hiding this comment.
Tests module is missing HashMap import (won’t compile).
HashMap usages inside mod tests don’t inherit the outer use. Add the import to the tests module.
#[cfg(test)]
mod tests {
+ use std::collections::HashMap;
use rocketmq_common::common::message::MessageConst;
use rocketmq_common::common::topic::TopicValidator;
use super::*;📝 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.
| #[cfg(test)] | |
| mod tests { | |
| use rocketmq_common::common::message::MessageConst; | |
| use rocketmq_common::common::topic::TopicValidator; | |
| use super::*; | |
| use crate::base::dispatch_request::DispatchRequest; | |
| use crate::config::message_store_config::MessageStoreConfig; | |
| #[cfg(test)] | |
| mod tests { | |
| use std::collections::HashMap; | |
| use rocketmq_common::common::message::MessageConst; | |
| use rocketmq_common::common::topic::TopicValidator; | |
| use super::*; | |
| use crate::base::dispatch_request::DispatchRequest; | |
| use crate::config::message_store_config::MessageStoreConfig; | |
| // …rest of tests… | |
| } |
🤖 Prompt for AI Agents
In rocketmq-store/src/queue.rs around lines 118 to 126, the tests module is
missing an import for HashMap causing compilation failures; add use
std::collections::HashMap; inside the #[cfg(test)] mod tests block (alongside
the existing use statements) so the HashMap type used in the tests is in scope.
| // Blank value fails | ||
| let mut props_blank = HashMap::new(); | ||
| props_blank.insert( | ||
| MessageConst::PROPERTY_INNER_MULTI_DISPATCH.into(), | ||
| " ".into(), | ||
| ); | ||
| props_blank.insert( | ||
| MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET.into(), | ||
| "2".into(), | ||
| ); | ||
| let dr = DispatchRequest { | ||
| properties_map: Some(props_blank), | ||
| ..Default::default() | ||
| }; | ||
| assert!(multi_dispatch_utils::check_multi_dispatch_queue(&cfg, &dr)); | ||
| } |
There was a problem hiding this comment.
Fix the assertion for the “Blank value fails” scenario to reflect the requirement.
The comment says “Blank value fails,” and the objectives require blank-or-empty to be invalid. After trimming in the implementation, this should assert false.
- assert!(multi_dispatch_utils::check_multi_dispatch_queue(&cfg, &dr));
+ assert!(!multi_dispatch_utils::check_multi_dispatch_queue(&cfg, &dr));📝 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.
| // Blank value fails | |
| let mut props_blank = HashMap::new(); | |
| props_blank.insert( | |
| MessageConst::PROPERTY_INNER_MULTI_DISPATCH.into(), | |
| " ".into(), | |
| ); | |
| props_blank.insert( | |
| MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET.into(), | |
| "2".into(), | |
| ); | |
| let dr = DispatchRequest { | |
| properties_map: Some(props_blank), | |
| ..Default::default() | |
| }; | |
| assert!(multi_dispatch_utils::check_multi_dispatch_queue(&cfg, &dr)); | |
| } | |
| // Blank value fails | |
| let mut props_blank = HashMap::new(); | |
| props_blank.insert( | |
| MessageConst::PROPERTY_INNER_MULTI_DISPATCH.into(), | |
| " ".into(), | |
| ); | |
| props_blank.insert( | |
| MessageConst::PROPERTY_INNER_MULTI_QUEUE_OFFSET.into(), | |
| "2".into(), | |
| ); | |
| let dr = DispatchRequest { | |
| properties_map: Some(props_blank), | |
| ..Default::default() | |
| }; | |
| assert!(!multi_dispatch_utils::check_multi_dispatch_queue(&cfg, &dr)); | |
| } |
🤖 Prompt for AI Agents
In rocketmq-store/src/queue.rs around lines 201 to 216, the test labeled "Blank
value fails" currently asserts true but the requirement is that blank or empty
property values are invalid after trimming; update the assertion to assert false
(i.e., expect the check_multi_dispatch_queue call to fail) so the test reflects
that a whitespace-only PROPERTY_INNER_MULTI_DISPATCH is rejected while other
props remain.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #3843
Brief Description
How Did You Test This Change?
Summary by CodeRabbit