[ISSUE #1870]⚡️Replace other structs attribute store_host with BrokerRuntime store_host #1871
Conversation
…Runtime store_host
WalkthroughThe pull request introduces modifications to the RocketMQ broker's runtime and processor components, focusing on the integration of a 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
|
|
🔊@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
rocketmq-rust-bot
requested review from
rocketmq-rust-bot and
RocketmqRustBot
![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
🧹 Nitpick comments (1)
rocketmq-broker/src/processor/send_message_processor.rs (1)
196-196: Consider removing redundant store_host storageWhile the addition of store_host is correct, it's currently stored in both the Inner struct and SendMessageProcessor struct. Consider storing it only in the Inner struct since that's where it's primarily used.
pub struct SendMessageProcessor<MS, TS> { inner: ArcMut<Inner<MS, TS>>, - store_host: SocketAddr, } impl<MS, TS> SendMessageProcessor<MS, TS> where MS: MessageStore, TS: TransactionalMessageService, { pub fn new( // ... other parameters ... store_host: SocketAddr, ) -> Self { Self { inner: ArcMut::new(Inner { // ... other fields ... store_host, }), - store_host, } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rocketmq-broker/src/broker_runtime.rs(4 hunks)rocketmq-broker/src/processor/ack_message_processor.rs(1 hunks)rocketmq-broker/src/processor/reply_message_processor.rs(1 hunks)rocketmq-broker/src/processor/send_message_processor.rs(1 hunks)rocketmq-broker/src/transaction/queue/transactional_message_bridge.rs(1 hunks)
🔇 Additional comments (5)
rocketmq-broker/src/transaction/queue/transactional_message_bridge.rs (1)
78-78: LGTM! Constructor parameter addition is clean.
The addition of the store_host parameter to the constructor aligns with the PR's objective of replacing the store_host attribute handling.
rocketmq-broker/src/processor/ack_message_processor.rs (1)
79-79: LGTM! Verify consistent usage of store_host throughout the processor.
The addition of the store_host parameter to the constructor is clean. The parameter is correctly used in message processing, particularly in the append_ack method.
✅ Verification successful
Let me analyze the usage of store_host across the processors. The search results show that store_host is consistently used in message processing across different processor modules:
ack_message_processor.rs: Setsstore_hostin message inner during ack processingreply_message_processor.rs: Setsstore_hostin message extension innersend_message_processor.rs: Setsstore_hostin multiple message processing pathsend_transaction_processor.rs: Copiesstore_hostfrom existing messagechange_invisible_time_processor.rs: Setsstore_hostin message inner
Let me verify the constructor pattern to ensure consistent initialization:
Let me try a different pattern to check the constructor implementations:
Let me check specifically for the store_host in struct definitions and new implementations with a different pattern:
LGTM! The store_host field is consistently implemented across processors
The verification confirms that store_host is properly and consistently used across the message processing chain:
- All relevant processors (
SendMessageProcessor,ReplyMessageProcessor,AckMessageProcessor,ChangeInvisibleTimeProcessor) correctly:- Define
store_host: SocketAddras a struct field - Accept
store_hostin theirnew()constructors - Use
store_hostconsistently when processing messages, setting it in message extensions
- Define
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of store_host in message processing
# Test: Search for store_host assignments in message processing
rg -A 2 'store_host\s*=' rocketmq-broker/src/processor/
Length of output: 3267
Script:
#!/bin/bash
# Search for store_host in struct definitions and new implementations
ast-grep --pattern 'impl $_ {
pub fn new($$$) -> Self {
$$$
store_host: $_
$$$
}
}'
Length of output: 117
Script:
#!/bin/bash
# Search for processor struct definitions and their new implementations
rg -A 5 'impl \w+Processor' rocketmq-broker/src/processor/
Length of output: 5031
Script:
#!/bin/bash
# Search for processor structs with store_host field and their new implementations
rg -B 3 -A 10 'store_host:.*SocketAddr' rocketmq-broker/src/processor/
Length of output: 10291
rocketmq-broker/src/broker_runtime.rs (3)
Line range hint 476-488: LGTM: Consistent store host propagation to message processors
The change correctly propagates the store_host from BrokerRuntime to the message processors, ensuring consistent broker host information across components.
571-571: LGTM: Consistent store host propagation to AckMessageProcessor
The change correctly propagates the store_host to the AckMessageProcessor, maintaining consistency with other processor initializations.
755-756: LGTM: Consistent store host propagation to TransactionalMessageBridge
The change correctly propagates the store_host to the TransactionalMessageBridge, ensuring consistent broker host information in transaction processing.
| @@ -73,10 +73,8 @@ where | |||
| broker_stats_manager: Arc<BrokerStatsManager>, | |||
| producer_manager: Option<Arc<ProducerManager>>, | |||
| transactional_message_service: ArcMut<TS>, | |||
| store_host: SocketAddr, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicated store_host field.
The store_host field is duplicated in both the main struct and the inner struct. This could lead to inconsistencies if the values diverge.
Consider this refactor to eliminate the duplication:
pub struct ReplyMessageProcessor<MS, TS> {
inner: Inner<MS, TS>,
- store_host: SocketAddr,
}
impl<MS, TS> ReplyMessageProcessor<MS, TS>
where
MS: MessageStore,
TS: TransactionalMessageService,
{
pub fn new(
// ... other parameters ...
store_host: SocketAddr,
) -> Self {
Self {
inner: Inner {
// ... other fields ...
store_host,
},
- store_host,
}
}
}Then update all usages of self.store_host to self.inner.store_host.
Also applies to: 91-91
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1871 +/- ##
=======================================
Coverage 28.47% 28.47%
=======================================
Files 475 475
Lines 66802 66798 -4
=======================================
Hits 19024 19024
+ Misses 47778 47774 -4 ☔ View full report in Codecov by Sentry. |
mxsm
changed the title
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1870
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
store_hostparameter across multiple processors and theTransactionalMessageBridge, allowing for more straightforward initialization and improved contextual information during message handling.Bug Fixes
AckMessageProcessor,ReplyMessageProcessor, andSendMessageProcessorby removing unnecessary parsing logic from the broker configuration.Documentation
store_hostparameter in relevant constructors.