[ISSUE #1200]⚡️Optimize CheetahString for rocketmq-broker and rocketmq-store🎨 #1201
Conversation
WalkthroughThe changes in this pull request primarily focus on transitioning from standard Rust string types ( Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@rocketmq-rust-bot has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 🎉 @mxsm. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
|
🚀Thanks for your contribution 🎉 @mxsm. CodeRabbit(AI) will review your code first 🔥 |
|
🚀Thanks for your contribution 🎉 @mxsm. 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: 17
🧹 Outside diff range and nitpick comments (69)
rocketmq-broker/src/processor/pop_inflight_message_counter.rs (1)
20-24: Consider implementing metrics for string optimizationSince this is part of a broader effort to optimize string handling with
CheetahString, consider adding performance metrics collection to validate the optimization:
- Message processing latency
- Memory usage patterns
- String allocation/deallocation counts
This would help quantify the benefits of migrating from
&strtoCheetahString.rocketmq-store/src/kv/compaction_store.rs (2)
33-34: Add documentation explaining CheetahString benefits.While the type changes align with the optimization goals, it would be helpful to document why CheetahString is more suitable here than standard Rust string types.
Implementation pending in CompactionStore
The
get_messagemethod inCompactionStoreis intentionally returningNoneas it's a pending implementation. This is evidenced by:
- The method is part of a larger message store system where other components like
DefaultMessageStoredelegate toCompactionStorefor certain message retrievals- Other implementations of
get_messagein the codebase (e.g., inCommitLog,DefaultMessageStore) are fully implemented with proper message retrieval logic- The
CompactionStorestruct itself is minimal, suggesting it's a work in progress🔗 Analysis chain
Line range hint
33-41: Verify implementation and usage.The method currently returns
None. Please verify:
- If this is intended behavior or if implementation is pending
- That all callers have been updated to use CheetahString
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to get_message to ensure they've been updated rg -A 2 "get_message.*group.*topic" --type rust # Find potential string literals being passed where CheetahString is expected ast-grep --pattern 'get_message($$$, "', 'get_message("$$$", "'Length of output: 183
Script:
#!/bin/bash # Search for any calls to get_message method rg "get_message" --type rust -A 3 # Find the trait or interface definition for this method ast-grep --pattern 'fn get_message($$$) -> Option<GetMessageResult>' # Look for any tests or implementation details fd "test" --extension rs --exec rg "get_message" {}Length of output: 56394
rocketmq-remoting/src/protocol/body/group_list.rs (1)
38-40: LGTM! Consider adding validation in the future.The setter correctly takes ownership of the new HashSet. As this is part of a protocol implementation, consider adding validation logic in the future to ensure group names meet any protocol-specific requirements.
rocketmq-common/src/common/statistics/state_getter.rs (2)
Line range hint
20-36: Update documentation to reflect CheetahString parametersThe documentation still refers to parameters as "string slice" which is no longer accurate since we're using
CheetahString./// # Arguments /// - /// * `instance_id` - A string slice that holds the ID of the instance. - /// * `group` - A string slice that holds the group of the instance. - /// * `topic` - A string slice that holds the topic of the instance. + /// * `instance_id` - A CheetahString reference that holds the ID of the instance. + /// * `group` - A CheetahString reference that holds the group of the instance. + /// * `topic` - A CheetahString reference that holds the topic of the instance.
37-42: LGTM! Method signature updated correctlyThe change from
&strto&CheetahStringaligns with the optimization goals for string handling in the broker and store components.Consider documenting the performance characteristics and memory layout differences between
CheetahStringand standard Rust strings in the crate's documentation to help future maintainers understand the optimization benefits.rocketmq-store/src/base/message_arriving_listener.rs (2)
Line range hint
36-43: Update method documentation to reflect parameter type changesThe method documentation needs to be updated to accurately reflect:
- The
topicparameter now accepts aCheetahStringreference- The
tags_codeparameter is now optional- The
propertiesparameter now usesCheetahStringfor both keys and valuesApply this diff to update the documentation:
/// # Arguments /// - /// * `topic` - A string that holds the topic of the message. + /// * `topic` - A CheetahString that holds the topic of the message. /// * `queue_id` - An i32 that holds the id of the queue where the message is placed. /// * `logic_offset` - An i64 that represents the logical offset of the message in the queue. - /// * `tags_code` - An i64 that represents the tags associated with the message. + /// * `tags_code` - An Option<i64> that represents the tags associated with the message, if present. /// * `msg_store_time` - An i64 that represents the time when the message was stored. /// * `filter_bit_map` - A Vec<u8> that represents the filter bit map for the message. /// * `properties` - An Option containing a reference to a HashMap<String, String> that holds - /// the properties of the message. + /// the properties of the message, using CheetahString for both keys and values.
Line range hint
36-43: Well-structured type changes for better flexibility and consistencyThe changes demonstrate good architectural decisions:
- Making
tags_codeoptional improves flexibility by properly handling cases where tags aren't present- Consistent use of
CheetahStringacross all string fields aligns with the optimization goals- The changes maintain backward compatibility by keeping the same method name and non-string parameter types
Consider documenting the performance benefits of
CheetahStringin the module-level documentation to help future maintainers understand the rationale behind using this custom string type over standard Rust strings.rocketmq-remoting/src/protocol/heartbeat/consumer_data.rs (1)
32-32: Consider documenting CheetahString performance characteristicsAs part of the optimization effort, it would be valuable to:
- Document the performance benefits of CheetahString over String
- Add benchmarks comparing String vs CheetahString operations
- Consider adding migration guidelines for other parts of the codebase
rocketmq-broker/src/long_polling/notify_message_arriving_listener.rs (1)
Line range hint
47-57: Consider adding documentation for the CheetahString usage.The method parameters would benefit from documentation explaining the purpose of using
CheetahStringand any specific requirements or guarantees it provides.Add documentation like this:
+ /// Notifies the system about a new message arriving + /// + /// # Parameters + /// * `topic` - The topic identifier as a CheetahString for optimized string operations + /// * `queue_id` - The queue identifier + /// * `logic_offset` - The logical offset in the queue + /// * `tags_code` - Optional tags code for message filtering + /// * `msg_store_time` - Message storage timestamp + /// * `filter_bit_map` - Optional bitmap for message filtering + /// * `properties` - Optional message properties using CheetahString for keys and values fn arriving( &self, topic: &CheetahString,rocketmq-store/src/queue/consume_queue_ext.rs (1)
48-49: Consider consistent string type usage in path handlingWhile the path construction is correct, there's an opportunity to improve consistency:
- Line 48-49 correctly uses
as_str()for PathBuf construction- However, on line 53 (in context), we convert back to String using
to_string_lossy().to_string()Consider using a consistent approach for path handling throughout the method, possibly by implementing a trait for CheetahString that handles path conversions more efficiently.
Example trait implementation (to be added in the CheetahString crate):
use std::path::Path; trait IntoPath { fn into_path(&self) -> PathBuf; } impl IntoPath for CheetahString { fn into_path(&self) -> PathBuf { PathBuf::from(self.as_str()) } }rocketmq-broker/src/filter/consumer_filter_data.rs (2)
73-85: Consider adding input validation and tests for setter methodsThe setter methods have been updated for
CheetahString, but there are two concerns:
- No input validation is performed
- Test coverage is missing
Consider adding:
- Length limits or format validation if applicable
- Unit tests for the setters
Would you like me to help with:
- Implementing input validation logic
- Generating unit tests for these methods
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-73: rocketmq-broker/src/filter/consumer_filter_data.rs#L73
Added line #L73 was not covered by tests
[warning] 77-77: rocketmq-broker/src/filter/consumer_filter_data.rs#L77
Added line #L77 was not covered by tests
[warning] 81-81: rocketmq-broker/src/filter/consumer_filter_data.rs#L81
Added line #L81 was not covered by tests
[warning] 85-85: rocketmq-broker/src/filter/consumer_filter_data.rs#L85
Added line #L85 was not covered by tests
Line range hint
28-85: Consider documenting CheetahString performance characteristicsWhile the transition to
CheetahStringis consistent throughout the file, it would be beneficial to:
- Document the performance benefits of
CheetahStringover standardString- Add comments explaining any specific requirements or constraints when using
CheetahString- Consider adding examples in the struct-level documentation
This will help future maintainers understand the rationale behind using
CheetahString.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 41-41: rocketmq-broker/src/filter/consumer_filter_data.rs#L41
Added line #L41 was not covered by tests
[warning] 45-45: rocketmq-broker/src/filter/consumer_filter_data.rs#L45
Added line #L45 was not covered by tests
[warning] 49-49: rocketmq-broker/src/filter/consumer_filter_data.rs#L49
Added line #L49 was not covered by tests
[warning] 53-53: rocketmq-broker/src/filter/consumer_filter_data.rs#L53
Added line #L53 was not covered by testsrocketmq-store/src/base/dispatch_request.rs (1)
Line range hint
50-70: Consider adding documentation for default valuesThe default implementation looks good with proper initialization of
CheetahStringfields. Consider adding documentation to explain the significance of these default values, especially for fields likebuffer_size: -1andmsg_base_offset: -1.impl Default for DispatchRequest { + /// Creates a new DispatchRequest with default values. + /// + /// # Notes + /// - buffer_size: -1 indicates uninitialized buffer + /// - msg_base_offset: -1 indicates no base offset + /// - batch_size: 1 indicates single message fn default() -> Self {rocketmq-broker/src/subscription/manager/subscription_group_manager.rs (1)
Line range hint
166-195: Inconsistency: get_forbidden methods still use &str.The methods
get_forbiddenandget_forbidden_internalhaven't been updated to use CheetahString, which is inconsistent with the rest of the changes in this file.Consider updating these methods to maintain consistency:
- pub fn get_forbidden(&self, group: &str, topic: &str, forbidden_index: i32) -> bool { + pub fn get_forbidden(&self, group: &CheetahString, topic: &CheetahString, forbidden_index: i32) -> bool { let topic_forbidden = self.get_forbidden_internal(group, topic); let bit_forbidden = 1 << forbidden_index; (topic_forbidden & bit_forbidden) == bit_forbidden } - pub fn get_forbidden_internal(&self, group: &str, topic: &str) -> i32 { + pub fn get_forbidden_internal(&self, group: &CheetahString, topic: &CheetahString) -> i32 {rocketmq-common/src/common/message/message_ext_broker_inner.rs (1)
69-71: Document the relationship betweentopic()andget_topic()The implementation looks correct, but having two methods (
topic()andget_topic()) that return different types (&strvs&CheetahString) for the same data could be confusing to users.Consider:
- Adding documentation to clarify the difference between these methods
- Marking
topic()as deprecated if it's being phased out in favor ofget_topic()+ /// Returns a reference to the topic as a CheetahString. + /// This method aligns with the MessageTrait implementation and is preferred over `topic()`. pub fn get_topic(&self) -> &CheetahString { self.message_ext_inner.get_topic() } + /// Returns a reference to the topic as a str. + /// Note: This method is maintained for backward compatibility and might be deprecated in favor of `get_topic()`. pub fn topic(&self) -> &str { self.message_ext_inner.topic() }rocketmq-store/src/queue/batch_consume_queue.rs (3)
Line range hint
91-108: Refactor duplicated path construction logicThe path construction logic is duplicated between the if and else branches. Consider extracting it to reduce code duplication.
pub fn new( topic: CheetahString, queue_id: i32, store_path: CheetahString, mapped_file_size: usize, subfolder: Option<CheetahString>, message_store_config: Arc<MessageStoreConfig>, ) -> Self { let commit_log_size = message_store_config.mapped_file_size_commit_log; - let mapped_file_queue = if let Some(subfolder) = subfolder { - let queue_dir = PathBuf::from(store_path.as_str()) - .join(topic.as_str()) - .join(queue_id.to_string()) - .join(subfolder.as_str()); - MappedFileQueue::new( - queue_dir.to_string_lossy().to_string(), - mapped_file_size as u64, - None, - ) - } else { - let queue_dir = PathBuf::from(store_path.as_str()) - .join(topic.as_str()) - .join(queue_id.to_string()); - MappedFileQueue::new( - queue_dir.to_string_lossy().to_string(), - mapped_file_size as u64, - None, - ) - }; + let mut queue_dir = PathBuf::from(store_path.as_str()) + .join(topic.as_str()) + .join(queue_id.to_string()); + + if let Some(subfolder) = subfolder { + queue_dir = queue_dir.join(subfolder.as_str()); + } + + let mapped_file_queue = MappedFileQueue::new( + queue_dir.to_string_lossy().to_string(), + mapped_file_size as u64, + None, + );
200-202: Implementation needed for get_topic methodThe return type change from
&strto&CheetahStringis correct, but the implementation is missing.Would you like me to help implement this method? A simple implementation would be:
fn get_topic(&self) -> &CheetahString { - todo!() + &self.topic }
Line range hint
1-308: Consider documenting CheetahString performance characteristicsThe transition to
CheetahStringappears to be part of a broader optimization strategy. To help future maintainers:
- Consider adding documentation about the performance benefits of
CheetahStringoverString- Document any memory/performance tradeoffs that influenced this design decision
- Add benchmarks comparing the old and new implementations
rocketmq-remoting/src/protocol/subscription/subscription_group_config.rs (2)
150-154: Consider updating the group_name getter return type.While the implementation correctly uses
CheetahStringfor storage and mutation, thegroup_name()getter still returns&str. Consider updating it to return&CheetahStringfor consistency with the new type system, unless there's a specific reason to convert to&strat the API boundary.- pub fn group_name(&self) -> &str { - &self.group_name + pub fn group_name(&self) -> &CheetahString { + &self.group_name }Also applies to: 202-204
Line range hint
235-249: LGTM! Consider adding CheetahString-specific test cases.The test updates correctly handle the transition to
CheetahString. However, consider adding test cases that specifically verify CheetahString-specific behavior, such as:
- Creation from different string types
- Memory efficiency compared to standard String
- Any special CheetahString features or constraints
Also applies to: 270-270
rocketmq-broker/src/long_polling/long_polling_service/pull_request_hold_service.rs (2)
120-127: Consider enhancing debug logging for better observability.The commented-out logging could be valuable for troubleshooting. Consider:
- Uncommenting it with an appropriate log level (debug/trace instead of info)
- Including additional context like max_offset
- /*info!( - "check hold request, topic: {}, queue_id: {}", - topic, queue_id - );*/ + debug!( + "Checking hold request - topic: {}, queue_id: {}, max_offset: {}", + topic, queue_id, max_offset + );
Line range hint
120-143: Consider enhancing error handling and documentation.While the type transition looks good, there are opportunities for architectural improvements:
- The
unwrap()on line 121 for parsing queue_id could panic. Consider proper error handling:- let queue_id = key_parts[1].parse::<i32>().unwrap(); + let queue_id = match key_parts[1].parse::<i32>() { + Ok(id) => id, + Err(e) => { + warn!("Invalid queue_id format in key {}: {}", key, e); + continue; + } + };
- Consider adding documentation for the CheetahString type usage, especially regarding its benefits and any performance implications.
rocketmq-broker/src/client/manager/consumer_manager.rs (1)
258-258: Consider optimizing query_topic_consume_by_who return typeThe method returns a
HashSet<CheetahString>which involves cloning strings into the result set. Consider whether returning references would be more efficient:-pub fn query_topic_consume_by_who(&self, topic: &CheetahString) -> HashSet<CheetahString> { +pub fn query_topic_consume_by_who(&self, topic: &CheetahString) -> HashSet<&CheetahString> { let mut groups = HashSet::new(); for (group, consumer_group_info) in self.consumer_table.read().iter() { if consumer_group_info.find_subscription_data(topic).is_some() { - groups.insert(group.clone()); + groups.insert(group); } } groups }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 258-258: rocketmq-broker/src/client/manager/consumer_manager.rs#L258
Added line #L258 was not covered by testsrocketmq-common/src/common/message/message_single.rs (2)
Line range hint
199-201: Consider returningOption<&CheetahString>for consistencyThe
transaction_id()method currently returnsOption<&str>while the field isOption<CheetahString>. For consistency with the codebase's transition toCheetahString, consider updating the return type.- pub fn transaction_id(&self) -> Option<&str> { - self.transaction_id.as_deref() + pub fn transaction_id(&self) -> Option<&CheetahString> { + self.transaction_id.as_ref() }
Line range hint
89-91: Optimize HashMap key lookupSince the properties HashMap uses
CheetahStringas its key type, converting tostrmight be unnecessary.- self.properties.remove(name.into().as_str()); + self.properties.remove(&name.into());rocketmq-broker/src/processor/admin_broker_processor/offset_request_handler.rs (1)
Line range hint
151-186: Consider extracting common RPC handling logicBoth methods contain similar RPC communication and error handling patterns. Consider extracting this logic into a reusable helper function to reduce code duplication.
Example refactor:
async fn handle_offset_rpc<T, R>( &self, request_code: RequestCode, request_header: T, error_msg: &str, ) -> Result<R, RemotingCommand> where T: Clone, R: RemotingResponseHeader, { let rpc_request = RpcRequest::new(request_code.to_i32(), request_header, None); let rpc_response = self .inner .broker_out_api .rpc_client() .invoke(rpc_request, self.inner.broker_config.forward_timeout) .await .map_err(|e| { RemotingCommand::create_response_command_with_code(ResponseCode::SystemError) .set_remark(format!("{}", e)) })?; rpc_response.get_header::<R>().ok_or_else(|| { RemotingCommand::create_response_command_with_code(ResponseCode::SystemError) .set_remark(error_msg.to_string()) }) }Also applies to: 220-256
rocketmq-client/src/consumer/mq_consumer_inner.rs (2)
Line range hint
133-140: Consider replacing panic with Result return typeThe current implementation panics when
default_mqpush_consumer_implis None, which could be problematic in production. Consider changing the trait to return aResult<CheetahString>instead.- fn group_name(&self) -> CheetahString { + fn group_name(&self) -> Result<CheetahString> { if let Some(ref default_mqpush_consumer_impl) = self.default_mqpush_consumer_impl { if let Some(default_mqpush_consumer_impl) = default_mqpush_consumer_impl.upgrade() { - return MQConsumerInner::group_name(default_mqpush_consumer_impl.as_ref()); + return Ok(MQConsumerInner::group_name(default_mqpush_consumer_impl.as_ref())); } } - panic!("default_mqpush_consumer_impl is None"); + Err(Error::ConsumerNotInitialized) }This change would:
- Make the error case explicit in the method signature
- Allow callers to handle the error case gracefully
- Follow Rust's error handling best practices
41-41: Consider documenting CheetahString performance characteristicsThe transition to
CheetahStringis a significant architectural change. Consider:
- Adding documentation about the performance characteristics and memory usage of
CheetahString- Documenting any conversion costs between
StringandCheetahString- Adding benchmarks to validate the optimization
Also applies to: 133-140
rocketmq-broker/src/topic/manager/topic_queue_mapping_manager.rs (1)
Line range hint
197-211: LGTM! But there's an inconsistency withget_topic_queue_mapping.The
deletemethod implementation is correct and thread-safe. However, there's an inconsistency in the API design wheredeleteaccepts&CheetahStringwhileget_topic_queue_mappingstill accepts&str.Consider updating
get_topic_queue_mappingto maintain API consistency:- pub fn get_topic_queue_mapping(&self, topic: &str) -> Option<TopicQueueMappingDetail> { + pub fn get_topic_queue_mapping(&self, topic: &CheetahString) -> Option<TopicQueueMappingDetail> { - self.topic_queue_mapping_table.lock().get(topic).cloned() + self.topic_queue_mapping_table.lock().get(topic).cloned() }rocketmq-store/src/log_file.rs (1)
Line range hint
194-394: Consider documenting CheetahString performance characteristics and migration guidelinesSince this is a significant change in the string handling strategy:
- Consider adding documentation about CheetahString's performance characteristics and memory usage patterns
- Provide migration guidelines for downstream implementations of this trait
- Consider adding benchmarks to validate the performance improvements
rocketmq-broker/src/client/consumer_group_info.rs (1)
247-249: Add test coverage for find_subscription_data.While the method is indirectly tested through
update_subscription, it would be beneficial to add direct test cases forfind_subscription_datato ensure proper functionality with CheetahString.Would you like me to help generate test cases? Here's a suggested implementation:
#[test] fn test_find_subscription_data() { let consumer_group_info = ConsumerGroupInfo::with_group_name("test_group"); // Create and add subscription data let mut sub_list = HashSet::new(); let topic: CheetahString = "test_topic".into(); let subscription_data = SubscriptionData { topic: topic.clone(), sub_string: "test_sub".into(), ..Default::default() }; sub_list.insert(subscription_data.clone()); // Update subscription consumer_group_info.update_subscription(&sub_list); // Test finding existing subscription assert_eq!( consumer_group_info.find_subscription_data(&topic), Some(subscription_data) ); // Test finding non-existent subscription assert_eq!( consumer_group_info.find_subscription_data(&"non_existent".into()), None ); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-247: rocketmq-broker/src/client/consumer_group_info.rs#L247
Added line #L247 was not covered by testsrocketmq-broker/src/transaction/queue/transactional_message_bridge.rs (2)
99-101: Consider caching the static consumer group CheetahStringThe static consumer group string is converted to CheetahString on every method call. Consider caching this value as a static or const to avoid repeated conversions.
+ static CONSUMER_GROUP: Lazy<CheetahString> = Lazy::new(|| + CheetahString::from_static_str(TransactionalMessageUtil::build_consumer_group()) + ); pub(crate) fn fetch_consume_offset(&self, mq: &MessageQueue) -> i64 { - let group = CheetahString::from_static_str(TransactionalMessageUtil::build_consumer_group()); + let group = &CONSUMER_GROUP;
Line range hint
244-255: Consider adding error logging for topic creation failureWhile the implementation is correct, consider adding error logging when topic creation fails to help with debugging in production environments.
pub fn select_topic_config(&mut self, topic: &CheetahString) -> Option<TopicConfig> { let mut topic_config = self.topic_config_manager.select_topic_config(topic); if topic_config.is_none() { topic_config = self .topic_config_manager .create_topic_in_send_message_back_method( topic, 1, PermName::PERM_WRITE | PermName::PERM_READ, false, 0, ); + if topic_config.is_none() { + error!("Failed to create topic config for topic: {}", topic); + } } topic_config }rocketmq-broker/src/processor/consumer_manage_processor.rs (1)
178-179: LGTM with a minor suggestion for error messagesThe transition to
as_ref()is correct. However, consider using format strings for error messages consistently throughout the method.Example improvement for error messages:
- .set_remark(format!("subscription group not exist, {}", group)), + .set_remark(format!("Subscription group '{}' does not exist", group)),rocketmq-store/src/queue/local_file_consume_queue_store.rs (2)
106-113: Consider consolidating path construction logic.The repeated pattern of converting paths to
CheetahStringcould be refactored into a helper function to reduce duplication and improve maintainability.+ fn to_cheetah_path(path: &str) -> CheetahString { + CheetahString::from_string(path.to_string()) + }
544-546: Consider adding documentation for lifecycle management.The
get_life_cyclefunction is a crucial part of the queue management system but lacks documentation explaining its purpose and contract.Add documentation:
+ /// Returns the consume queue for the given topic and queue ID, creating it if it doesn't exist. + /// This function is part of the queue lifecycle management system and ensures that queues are + /// properly initialized and maintained throughout their lifecycle. fn get_life_cycle(&self, topic: &CheetahString, queue_id: i32) -> ArcConsumeQueue {rocketmq-broker/src/topic/manager/topic_config_manager.rs (2)
Line range hint
335-379: Consider improving error handling for lock acquisition.While the type change to
&CheetahStringis good, the lock acquisition usingtry_lock_forcould benefit from better error handling. Currently, if the lock acquisition fails, it silently returnsNonewithout logging the failure.Consider adding error logging:
let (topic_config, create_new) = if let Some(_lock) = self .topic_config_table_lock .try_lock_for(Duration::from_secs(3)) { // ... existing code ... } else { + warn!( + "Failed to acquire lock for topic creation after 3s: {}", + topic + ); (None, false) };
Line range hint
409-424: Consider atomic check-and-remove operation.While the logging is good, there's a potential race condition between checking existence and removal. The
remove_topic_configoperation should be atomic with respect to other modifications of the topic config table.Consider using a single atomic operation:
- let old = self.remove_topic_config(topic); - if let Some(old) = old { + let mut lock = self.topic_config_table.lock(); + if let Some(old) = lock.remove(topic) { info!("delete topic config OK, topic: {:?}", old); let state_machine_version = if let Some(message_store) = self.message_store.as_ref() { message_store.get_state_machine_version() } else { 0 }; self.data_version .mut_from_ref() .next_version_with(state_machine_version); self.persist();rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs (5)
338-342: Consider consolidating retry topic creation logicThe creation of retry topics (v1 and v2) follows a similar pattern and could be extracted into a helper method to improve code maintainability and reduce duplication.
Consider refactoring to:
fn create_retry_topic(&self, topic: &CheetahString, group: &str, is_v2: bool) -> CheetahString { if is_v2 { CheetahString::from_string(KeyBuilder::build_pop_retry_topic(topic, group, true)) } else { CheetahString::from_string(KeyBuilder::build_pop_retry_topic_v1(topic, group)) } }Also applies to: 351-353
Line range hint
439-456: Enhance error handling for topic statistics retrievalWhile the implementation is functionally correct, consider adding more detailed error handling for edge cases:
- Invalid queue numbers
- Store access failures
- Timestamp retrieval errors
Consider wrapping store operations in a
Resulttype and propagating errors with context:fn get_queue_stats(&self, topic: &CheetahString, queue_id: i32) -> Result<TopicOffset, String> { let min = self.default_message_store .get_min_offset_in_queue(topic, queue_id) .map_err(|e| format!("Failed to get min offset: {}", e))?; // ... similar for max and timestamp Ok(TopicOffset::new(min, max, timestamp)) }
540-540: Add documentation for topic consumer query methodConsider adding documentation to explain:
- The purpose of querying both consume manager and offset manager
- Why groups are extended with offset groups
- The return value format
Add documentation like:
/// Queries all consumer groups that consume from the specified topic. /// This includes both active consumers from the consume manager and /// groups with committed offsets from the offset manager.
Line range hint
563-572: Add type annotations for better readabilityConsider adding type annotations to make the code more self-documenting and easier to maintain.
Consider adding:
let topics: Vec<CheetahString> = self.inner .consumer_offset_manager .which_topic_by_consumer(request_header.get_group());
Line range hint
579-588: Add error handling and logging for topic deletionThe topic deletion process involves multiple operations that could fail. Consider:
- Adding error handling for each operation
- Adding logging for successful/failed operations
- Making the method return a
ResulttypeConsider refactoring to:
fn delete_topic_in_broker(&mut self, topic: &CheetahString) -> Result<(), String> { info!("Deleting topic {} from broker", topic); self.inner.topic_config_manager.delete_topic_config(topic) .map_err(|e| format!("Failed to delete topic config: {}", e))?; // ... similar for other operations info!("Successfully deleted topic {} from broker", topic); Ok(()) }rocketmq-client/src/consumer/default_mq_push_consumer.rs (2)
Line range hint
116-118:⚠️ Breaking Change: Commented out message listener methodsThe removal of these getter methods could break existing code that depends on accessing the message listener configuration. Consider:
- Adding deprecation notices instead of commenting out
- Providing migration guidance for affected users
Suggested approach:
- Mark methods as deprecated instead of removing:
- /* pub fn message_listener(&self) -> &Option<ArcMut<MessageListener>> { - &self.message_listener - }*/ + #[deprecated( + since = "0.1", + note = "This method will be removed in future versions. Use register_message_listener_* methods instead." + )] + pub fn message_listener(&self) -> &Option<ArcMut<MessageListener>> { + &self.message_listener + }
- Document the migration path in the deprecation notice
Also applies to: 120-122
Line range hint
4-6: Add tests for CheetahString integrationThe TODO comment indicates missing tests. Given the significant type changes, comprehensive testing is crucial.
Would you like me to help generate unit tests for the CheetahString integration? The tests should cover:
- Consumer group getter/setter with CheetahString
- String conversion scenarios
- Error handling cases
rocketmq-store/src/queue/single_consume_queue.rs (2)
94-95: Consider optimizing string conversions in the constructorThe current implementation performs multiple string conversions that could be optimized:
- Path construction with multiple
as_str()calls- Nested
CheetahString::from_stringconversionConsider this optimization:
- let queue_dir = PathBuf::from(store_path.as_str()) - .join(topic.as_str()) + let queue_dir = PathBuf::from(store_path.as_ref()) + .join(topic.as_ref()) .join(queue_id.to_string()); - CheetahString::from_string(get_store_path_consume_queue_ext( - message_store_config.store_path_root_dir.as_str(), - )), + CheetahString::from(get_store_path_consume_queue_ext( + &message_store_config.store_path_root_dir, + )),Also applies to: 106-108
Line range hint
780-792: Optimize string formatting operationsThe current implementation creates unnecessary string allocations in queue operations through repeated
format!andCheetahString::from_stringcalls.Consider caching the formatted string or using a more efficient approach:
- CheetahString::from_string(format!("{}-{}", msg.topic(), msg.queue_id())), + // Option 1: Use a pre-allocated buffer + let mut buffer = String::with_capacity(msg.topic().len() + 8); + buffer.push_str(msg.topic()); + buffer.push('-'); + buffer.push_str(&msg.queue_id().to_string()); + CheetahString::from(buffer) + + // Option 2: Consider adding a method to construct from parts + // CheetahString::from_parts(msg.topic(), msg.queue_id())This optimization would be particularly beneficial in high-throughput scenarios.
rocketmq-store/src/queue.rs (1)
606-606: Consider explicit lifetime annotations for get_topic methodThe
get_topicmethod returns a reference toCheetahString. Consider adding explicit lifetime annotations to make the relationship between the returned reference and the struct's lifetime clear.-fn get_topic(&self) -> &CheetahString; +fn get_topic<'a>(&'a self) -> &'a CheetahString;rocketmq-broker/src/broker_runtime.rs (2)
1110-1115: LGTM! Type safety improvements look good.The transition to
CheetahStringfor parameters improves type safety and consistency with the broader codebase changes.Consider adding documentation to explain the method's purpose and parameter requirements.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1110-1115: rocketmq-broker/src/broker_runtime.rs#L1110-L1115
Added lines #L1110 - L1115 were not covered by tests
1135-1153: Consider optimizing namespace wrapping operations.The current implementation wraps the namespace multiple times. Consider refactoring to reuse the wrapped strings:
fn online( &self, instance_id: &CheetahString, group: &CheetahString, topic: &CheetahString, ) -> bool { if self .topic_config_manager .topic_config_table() .lock() .contains_key(topic) { - let topic_full_name = - CheetahString::from_string(NamespaceUtil::wrap_namespace(instance_id, topic)); + let topic_full_name = CheetahString::from_string(NamespaceUtil::wrap_namespace(instance_id, topic)); + let group_full_name = CheetahString::from_string(NamespaceUtil::wrap_namespace(instance_id, group)); self.consumer_manager .find_subscription_data( - CheetahString::from_string(NamespaceUtil::wrap_namespace(instance_id, group)) - .as_ref(), + group_full_name.as_ref(), topic_full_name.as_ref(), ) .is_some() } else { self.consumer_manager .find_subscription_data(group, topic) .is_some() } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1135-1140: rocketmq-broker/src/broker_runtime.rs#L1135-L1140
Added lines #L1135 - L1140 were not covered by tests
[warning] 1147-1148: rocketmq-broker/src/broker_runtime.rs#L1147-L1148
Added lines #L1147 - L1148 were not covered by tests
[warning] 1151-1153: rocketmq-broker/src/broker_runtime.rs#L1151-L1153
Added lines #L1151 - L1153 were not covered by testsrocketmq-broker/src/processor/send_message_processor.rs (4)
903-905: Consider making DLQ topic name construction more configurable.The DLQ topic name is constructed using a hardcoded prefix. Consider making this configurable through broker configuration to support different naming conventions in various deployment scenarios.
911-914: Consider making DLQ permissions configurable.The DLQ topic is created with hardcoded permissions (PERM_WRITE | PERM_READ) and a fixed queue number. Consider making these configurable through broker configuration for better flexibility.
- PermName::PERM_WRITE | PermName::PERM_READ, + self.inner.broker_config.dlq_permissions,
1136-1139: Consider making retry topic permissions configurable.Similar to DLQ topic creation, the retry topic is created with hardcoded permissions. Consider making these configurable through broker configuration.
- PermName::PERM_WRITE | PermName::PERM_READ, + self.inner.broker_config.retry_topic_permissions,
Line range hint
856-1136: Overall code quality is good with room for improvement.The changes demonstrate good practices in:
- Consistent use of CheetahString for type safety
- Comprehensive error handling
- Appropriate logging levels
Consider creating a configuration struct to centralize and manage various topic-related settings (permissions, queue numbers, etc.) for better maintainability.
rocketmq-store/src/message_store/default_message_store.rs (2)
1119-1123: Consider consolidating path construction logic.While the current path handling is functional, consider extracting the common path construction pattern into a helper function to reduce code duplication and improve maintainability.
+ fn construct_queue_path(root_dir: &str, queue_type: &str, topic: &str) -> PathBuf { + PathBuf::from(match queue_type { + "consume" => get_store_path_consume_queue(root_dir), + "consume_ext" => get_store_path_consume_queue_ext(root_dir), + "batch" => get_store_path_batch_consume_queue(root_dir), + _ => panic!("Unknown queue type"), + }).join(topic) + }
1343-1346: Consider optimizing string conversions.While the current implementation works, consider creating the
CheetahStringinstances outside the loop to avoid repeated allocations, especially if the strings are reused.+ let queue_names: Vec<CheetahString> = queues.iter().map(|&q| CheetahString::from_slice(q)).collect(); for i in 0..queues.len() { - let queue_name = CheetahString::from_slice(queues[i]); + let queue_name = &queue_names[i];Also applies to: 1355-1355
rocketmq-store/src/queue/queue_offset_operator.rs (4)
57-62: Ensure consistent parameter passing by using referencesIn the
increase_queue_offsetmethod,topic_queue_keyis passed by value. To maintain consistency and reduce potential cloning overhead, pass it as a reference&CheetahString, similar to other methods.Proposed changes:
-pub fn increase_queue_offset(&self, topic_queue_key: CheetahString, message_num: i16) { +pub fn increase_queue_offset(&self, topic_queue_key: &CheetahString, message_num: i16) { let topic_queue_table = self.topic_queue_table.lock(); let mut table = topic_queue_table; - let entry = table.entry(topic_queue_key).or_insert(0); + let entry = table.entry(topic_queue_key.clone()).or_insert(0); *entry += message_num as i64; }
67-67: Avoid unnecessary cloning oftopic_queue_keyIn the
update_queue_offsetmethod,topic_queue_keyis cloned before insertion into the table. If you accepttopic_queue_keyby value instead of by reference, you can insert it directly without cloning.Modify the function signature and usage as follows:
-pub fn update_queue_offset(&self, topic_queue_key: &CheetahString, offset: i64) { +pub fn update_queue_offset(&self, topic_queue_key: CheetahString, offset: i64) { let topic_queue_table = self.topic_queue_table.lock(); let mut table = topic_queue_table; - table.insert(topic_queue_key.clone(), offset); + table.insert(topic_queue_key, offset); }
109-110: Optimizetopic_queue_keycreation to reduce allocationsIn the
removemethod,topic_queue_keyis created usingformat!, which allocates a newString. SinceCheetahString::fromcan accept&str, you can avoid the intermediateStringallocation by formatting directly into a&str.Refactor the code as follows:
-let topic_queue_key = CheetahString::from(format!("{}-{}", topic, queue_id)); +let topic_queue_key = CheetahString::from(&format!("{}-{}", topic, queue_id));
139-142: Simplify the parameter formatting inset_batch_topic_queue_tableThe method signature spans multiple lines, which can be condensed for better readability.
Refactor the method signature:
-pub fn set_batch_topic_queue_table( - &self, - batch_topic_queue_table: HashMap<CheetahString, i64>, -) { +pub fn set_batch_topic_queue_table(&self, batch_topic_queue_table: HashMap<CheetahString, i64>) { *self.batch_topic_queue_table.lock() = batch_topic_queue_table; }rocketmq-broker/src/offset/manager/consumer_offset_manager.rs (8)
84-84: Use consistent string types for comparisonConsider ensuring that
topic_at_groupandtopicare of the same type to avoid unnecessary conversions. Iftopic_at_groupis aCheetahString, you can usecontainsdirectly without convertingtopicto&str.Apply this diff:
- if topic_at_group.contains(topic.as_str()) { + if topic_at_group.contains(topic) {
102-102: OptimizeCheetahStringconversionUse
CheetahString::from_str(arr[1])instead of convertingarr[1]toStringfirst to improve performance.Apply this diff:
- let group = CheetahString::from_string(arr[1].to_string()); + let group = CheetahString::from_str(arr[1]);
117-118: Optimize key construction to reduce allocationsUsing
format!withCheetahString::from_stringmay introduce unnecessary allocations. Consider creating the key more efficiently by concatenatingCheetahStringinstances or using a custom method.Apply this diff:
- let key = - CheetahString::from_string(format!("{}{}{}", topic, TOPIC_GROUP_SEPARATOR, group)); + let key = { + let mut key = topic.clone(); + key.push_str(TOPIC_GROUP_SEPARATOR); + key.push_str(group.as_str()); + key + };
201-201: OptimizeCheetahStringconversionTo avoid unnecessary allocations, use
CheetahString::from_str(arr[0]).Apply this diff:
- let topic = CheetahString::from_string(arr[0].to_string()); + let topic = CheetahString::from_str(arr[0]);
243-244: Standardize parameter ordering in methodsThe order of
groupandtopicparameters varies betweencommit_pull_offsetandquery_then_erase_reset_offset. For consistency and readability, consider standardizing the parameter order across all methods.Apply this diff to
query_then_erase_reset_offset:- pub fn query_then_erase_reset_offset( - &self, - topic: &CheetahString, - group: &CheetahString, - queue_id: i32, - ) -> Option<i64> { + pub fn query_then_erase_reset_offset( + &self, + group: &CheetahString, + topic: &CheetahString, + queue_id: i32, + ) -> Option<i64> {Also applies to: 260-261
248-249: Optimize key construction incommit_pull_offsetSimilar to earlier, consider optimizing key creation to reduce allocations.
Apply this diff:
- let key = - CheetahString::from_string(format!("{}{}{}", topic, TOPIC_GROUP_SEPARATOR, group)); + let key = { + let mut key = topic.clone(); + key.push_str(TOPIC_GROUP_SEPARATOR); + key.push_str(group.as_str()); + key + };
286-287: Initializeret_mapwith appropriate capacityEnsure the initial capacity matches expected usage to optimize memory allocation.
292-293: OptimizeCheetahStringconversions inget_group_topic_mapUse
CheetahString::from_strto avoid unnecessary allocations.Apply this diff:
- let topic = CheetahString::from_string(arr[0].to_string()); - let group = CheetahString::from_string(arr[1].to_string()); + let topic = CheetahString::from_str(arr[0]); + let group = CheetahString::from_str(arr[1]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (47)
rocketmq-broker/src/broker_runtime.rs(2 hunks)rocketmq-broker/src/client/consumer_group_info.rs(1 hunks)rocketmq-broker/src/client/manager/consumer_manager.rs(9 hunks)rocketmq-broker/src/filter/consumer_filter_data.rs(4 hunks)rocketmq-broker/src/filter/manager/consumer_filter_manager.rs(3 hunks)rocketmq-broker/src/long_polling/long_polling_service/pull_request_hold_service.rs(1 hunks)rocketmq-broker/src/long_polling/notify_message_arriving_listener.rs(1 hunks)rocketmq-broker/src/offset/manager/consumer_offset_manager.rs(9 hunks)rocketmq-broker/src/processor/admin_broker_processor/consumer_request_handler.rs(1 hunks)rocketmq-broker/src/processor/admin_broker_processor/offset_request_handler.rs(4 hunks)rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs(8 hunks)rocketmq-broker/src/processor/client_manage_processor.rs(4 hunks)rocketmq-broker/src/processor/consumer_manage_processor.rs(6 hunks)rocketmq-broker/src/processor/default_pull_message_result_handler.rs(5 hunks)rocketmq-broker/src/processor/pop_inflight_message_counter.rs(1 hunks)rocketmq-broker/src/processor/pull_message_processor.rs(9 hunks)rocketmq-broker/src/processor/query_message_processor.rs(1 hunks)rocketmq-broker/src/processor/send_message_processor.rs(4 hunks)rocketmq-broker/src/subscription/manager/subscription_group_manager.rs(6 hunks)rocketmq-broker/src/topic/manager/topic_config_manager.rs(4 hunks)rocketmq-broker/src/topic/manager/topic_queue_mapping_manager.rs(2 hunks)rocketmq-broker/src/transaction/queue/transactional_message_bridge.rs(6 hunks)rocketmq-client/src/consumer/consumer_impl/default_mq_push_consumer_impl.rs(1 hunks)rocketmq-client/src/consumer/default_mq_push_consumer.rs(1 hunks)rocketmq-client/src/consumer/mq_consumer_inner.rs(2 hunks)rocketmq-client/src/factory/mq_client_instance.rs(1 hunks)rocketmq-common/src/common/message/message_ext.rs(1 hunks)rocketmq-common/src/common/message/message_ext_broker_inner.rs(1 hunks)rocketmq-common/src/common/message/message_single.rs(1 hunks)rocketmq-common/src/common/statistics/state_getter.rs(2 hunks)rocketmq-remoting/src/protocol/body/group_list.rs(1 hunks)rocketmq-remoting/src/protocol/heartbeat/consumer_data.rs(2 hunks)rocketmq-remoting/src/protocol/subscription/subscription_group_config.rs(9 hunks)rocketmq-store/src/base/dispatch_request.rs(2 hunks)rocketmq-store/src/base/message_arriving_listener.rs(1 hunks)rocketmq-store/src/kv/compaction_store.rs(2 hunks)rocketmq-store/src/log_file.rs(10 hunks)rocketmq-store/src/log_file/commit_log.rs(1 hunks)rocketmq-store/src/message_store/default_message_store.rs(11 hunks)rocketmq-store/src/queue.rs(16 hunks)rocketmq-store/src/queue/batch_consume_queue.rs(4 hunks)rocketmq-store/src/queue/consume_queue_ext.rs(2 hunks)rocketmq-store/src/queue/local_file_consume_queue_store.rs(18 hunks)rocketmq-store/src/queue/queue_offset_operator.rs(5 hunks)rocketmq-store/src/queue/single_consume_queue.rs(7 hunks)rocketmq-store/src/stats/broker_stats_manager.rs(3 hunks)rocketmq-store/src/timer/timer_message_store.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
rocketmq-broker/src/broker_runtime.rs
[warning] 1110-1115: rocketmq-broker/src/broker_runtime.rs#L1110-L1115
Added lines #L1110 - L1115 were not covered by tests
[warning] 1135-1140: rocketmq-broker/src/broker_runtime.rs#L1135-L1140
Added lines #L1135 - L1140 were not covered by tests
[warning] 1147-1148: rocketmq-broker/src/broker_runtime.rs#L1147-L1148
Added lines #L1147 - L1148 were not covered by tests
[warning] 1151-1153: rocketmq-broker/src/broker_runtime.rs#L1151-L1153
Added lines #L1151 - L1153 were not covered by tests
rocketmq-broker/src/client/consumer_group_info.rs
[warning] 247-247: rocketmq-broker/src/client/consumer_group_info.rs#L247
Added line #L247 was not covered by tests
rocketmq-broker/src/client/manager/consumer_manager.rs
[warning] 86-90: rocketmq-broker/src/client/manager/consumer_manager.rs#L86-L90
Added lines #L86 - L90 were not covered by tests
[warning] 96-97: rocketmq-broker/src/client/manager/consumer_manager.rs#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 114-114: rocketmq-broker/src/client/manager/consumer_manager.rs#L114
Added line #L114 was not covered by tests
[warning] 121-121: rocketmq-broker/src/client/manager/consumer_manager.rs#L121
Added line #L121 was not covered by tests
[warning] 127-127: rocketmq-broker/src/client/manager/consumer_manager.rs#L127
Added line #L127 was not covered by tests
[warning] 143-144: rocketmq-broker/src/client/manager/consumer_manager.rs#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 149-150: rocketmq-broker/src/client/manager/consumer_manager.rs#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 159-159: rocketmq-broker/src/client/manager/consumer_manager.rs#L159
Added line #L159 was not covered by tests
[warning] 165-166: rocketmq-broker/src/client/manager/consumer_manager.rs#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 173-173: rocketmq-broker/src/client/manager/consumer_manager.rs#L173
Added line #L173 was not covered by tests
[warning] 195-195: rocketmq-broker/src/client/manager/consumer_manager.rs#L195
Added line #L195 was not covered by tests
[warning] 205-205: rocketmq-broker/src/client/manager/consumer_manager.rs#L205
Added line #L205 was not covered by tests
[warning] 207-207: rocketmq-broker/src/client/manager/consumer_manager.rs#L207
Added line #L207 was not covered by tests
[warning] 258-258: rocketmq-broker/src/client/manager/consumer_manager.rs#L258
Added line #L258 was not covered by tests
rocketmq-broker/src/filter/consumer_filter_data.rs
[warning] 41-41: rocketmq-broker/src/filter/consumer_filter_data.rs#L41
Added line #L41 was not covered by tests
[warning] 45-45: rocketmq-broker/src/filter/consumer_filter_data.rs#L45
Added line #L45 was not covered by tests
[warning] 49-49: rocketmq-broker/src/filter/consumer_filter_data.rs#L49
Added line #L49 was not covered by tests
[warning] 53-53: rocketmq-broker/src/filter/consumer_filter_data.rs#L53
Added line #L53 was not covered by tests
[warning] 73-73: rocketmq-broker/src/filter/consumer_filter_data.rs#L73
Added line #L73 was not covered by tests
[warning] 77-77: rocketmq-broker/src/filter/consumer_filter_data.rs#L77
Added line #L77 was not covered by tests
[warning] 81-81: rocketmq-broker/src/filter/consumer_filter_data.rs#L81
Added line #L81 was not covered by tests
[warning] 85-85: rocketmq-broker/src/filter/consumer_filter_data.rs#L85
Added line #L85 was not covered by tests
rocketmq-broker/src/filter/manager/consumer_filter_manager.rs
[warning] 89-92: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L89-L92
Added lines #L89 - L92 were not covered by tests
[warning] 95-95: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L95
Added line #L95 was not covered by tests
[warning] 100-101: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L100-L101
Added lines #L100 - L101 were not covered by tests
🔇 Additional comments (100)
rocketmq-broker/src/processor/pop_inflight_message_counter.rs (1)
17-17: LGTM: Import statement is correctly placed
The import of CheetahString is properly placed after the license header and aligns with the codebase's transition to using this custom string type.
rocketmq-store/src/kv/compaction_store.rs (1)
17-18: LGTM! Import change aligns with optimization goals.
The addition of CheetahString import is consistent with the PR's objective to optimize string handling.
rocketmq-remoting/src/protocol/body/group_list.rs (5)
20-20: LGTM! Import statement is correctly placed.
The addition of CheetahString import aligns with the optimization objectives.
30-32: LGTM! Constructor implementation is clean and straightforward.
The constructor correctly handles the new type while maintaining its simplicity.
34-36: LGTM! Getter maintains proper borrowing semantics.
The implementation correctly returns a reference to the HashSet, preserving efficiency.
Line range hint 1-42: Verify consistent usage across the codebase.
As this is a type change in a protocol implementation, we should verify that all callers have been updated to use CheetahString.
#!/bin/bash
# Search for potential callers still using String with GroupList
rg -l "GroupList.*HashSet<String>" --type rust
# Search for direct usage of the methods
rg -l "get_group_list|set_group_list|GroupList::new" --type rust24-27: Verify CheetahString compatibility with HashSet and serde.
The change from String to CheetahString in the HashSet could affect serialization and hashing behavior.
✅ Verification successful
Based on the context gathered, I can now generate the final response since we've found that CheetahString is a type from an external crate (cheetah_string) and is widely used throughout the codebase with proper trait implementations for serialization, deserialization, and other necessary traits. The change from String to CheetahString in GroupList is safe.
Change from HashSet to HashSet is verified as safe
The CheetahString type is properly integrated into the RocketMQ codebase and implements all necessary traits required for HashSet usage (Hash, Eq) and serde traits (Serialize, Deserialize) as evidenced by its extensive usage in message properties, topic configs, and other core data structures throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if CheetahString implements necessary traits
ast-grep --pattern 'impl Hash for CheetahString'
ast-grep --pattern 'impl Eq for CheetahString'
ast-grep --pattern 'impl Serialize for CheetahString'
ast-grep --pattern 'impl Deserialize for CheetahString'
Length of output: 277
Script:
#!/bin/bash
# Let's check the CheetahString type definition and its implementations
rg -A 10 "struct CheetahString" --type rust
rg "#\[derive" -A 2 "CheetahString" --type rust
# Check if it's a newtype pattern around String
ast-grep --pattern 'struct CheetahString(String)'
# Look for any manual implementations for CheetahString
rg "impl.*CheetahString" --type rust -A 5
Length of output: 40132
Script:
#!/bin/bash
# Let's check the CheetahString type definition and its traits
rg "struct CheetahString" --type rust -A 5
rg "#\[derive" -A 2 -B 2 "struct CheetahString" --type rust
# Check if it's in common utils or types
fd -t f "string.rs|types.rs|common.rs" -x cat {}
Length of output: 4973
Script:
#!/bin/bash
# Let's check for CheetahString in rocketmq-common crate since it seems to be a common type
rg -g "*.rs" "CheetahString" rocketmq-common/src --type rust -B 2 -A 2
# Also check if it's defined in any utils module
fd -t f "utils.rs" -x cat {}
Length of output: 159260
Script:
#!/bin/bash
# Let's check for CheetahString in dependencies
rg -p "cheetah_string" Cargo.toml
# Look for CheetahString type definition in workspace
fd -t f "lib.rs" -x cat {}
Length of output: 17950
rocketmq-common/src/common/statistics/state_getter.rs (1)
17-17: Verify CheetahString usage across the codebase
Let's ensure this dependency is consistently used across the codebase and that we're not mixing string types unnecessarily.
rocketmq-remoting/src/protocol/heartbeat/consumer_data.rs (3)
20-20: LGTM! Import is correctly placed
The CheetahString import is properly organized with other external crate dependencies.
32-32: Verify CheetahString trait implementations and serde behavior
The change from String to CheetahString for group_name requires verification of trait implementations and serialization behavior.
✅ Verification successful
Based on the extensive search results showing CheetahString usage throughout the codebase, particularly the derive macros and trait implementations, I can now provide a final response:
CheetahString type change is safe and compatible
The change from String to CheetahString for group_name is safe as CheetahString implements all the required traits:
- Derives Debug, Clone, Serialize, Deserialize, Eq, PartialEq as evidenced by multiple struct definitions using CheetahString with the same derive macros
- Used extensively in HashMaps and HashSets indicating Hash trait implementation
- Widely used in serialization contexts across the codebase with serde traits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CheetahString implements required traits and check its serde implementation
# Check if CheetahString implements all required traits
ast-grep --pattern 'impl $trait for CheetahString'
# Check for any custom serde implementations for CheetahString
rg -A 5 'impl.*Serialize.*CheetahString|impl.*Deserialize.*CheetahString'
# Look for other ConsumerData serialization tests or examples
rg -A 5 'ConsumerData.*serialize|ConsumerData.*deserialize'
Length of output: 125463
Script:
#!/bin/bash
# Check CheetahString trait implementations
ast-grep --pattern 'impl $trait for CheetahString'
# Check for CheetahString struct definition and derive attributes
rg -A 5 'struct CheetahString|pub struct CheetahString'
# Look for CheetahString serialization tests
rg -A 5 'test.*CheetahString.*serde|test.*serde.*CheetahString'
Length of output: 176
Script:
#!/bin/bash
# Try to find CheetahString in a different location
fd -t f CheetahString.rs
# Look for CheetahString usage in tests
rg -g "*/tests/*" "CheetahString"
# Look for CheetahString type definition and implementations
rg "CheetahString" -A 5 -g "*.rs" --no-filename | grep -E "(struct|impl|derive|#\[derive)"
Length of output: 13938
Line range hint 40-50: Verify CheetahString's Hash implementation
The Hash implementation for ConsumerData relies on CheetahString's Hash implementation. Let's ensure it maintains consistent hashing behavior.
rocketmq-broker/src/long_polling/notify_message_arriving_listener.rs (1)
48-48: LGTM! Verify trait definition compatibility.
The change from &str to &CheetahString aligns with the optimization objectives and maintains consistency with other string parameters.
Let's verify the trait definition matches this implementation:
✅ Verification successful
Implementation matches trait definition perfectly
The implementation in notify_message_arriving_listener.rs correctly uses &CheetahString for the topic parameter, which exactly matches the trait definition in message_arriving_listener.rs. Both the trait and implementation are using the same type, and the change is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the MessageArrivingListener trait definition
ast-grep --pattern 'trait MessageArrivingListener {
$$$
fn arriving(&self, topic: $_, $$$) $$$
$$$
}'
Length of output: 109
Script:
#!/bin/bash
# Let's try a broader search for the trait definition using ripgrep
rg -U "trait MessageArrivingListener" -A 10
# Also search for any uses of this trait to understand the interface
rg "impl MessageArrivingListener"
Length of output: 1314
Script:
#!/bin/bash
# Let's check the trait definition's parameter type for topic
rg -U "fn arriving.*topic:.*," rocketmq-store/src/base/message_arriving_listener.rs
# Also search for any other implementations to ensure consistency
rg -U "fn arriving.*topic:.*," --type rust
Length of output: 130
Script:
#!/bin/bash
# Let's get the full content of both files to understand the interface and implementation
cat rocketmq-store/src/base/message_arriving_listener.rs
echo "---"
cat rocketmq-broker/src/long_polling/notify_message_arriving_listener.rs
Length of output: 4272
rocketmq-store/src/queue/consume_queue_ext.rs (3)
19-19: LGTM: Import added correctly
The CheetahString import is appropriately placed and necessary for the type changes.
42-44: Verify all constructor calls are updated
The parameter type changes to CheetahString are consistent with the struct changes. Let's verify all constructor calls have been updated accordingly.
#!/bin/bash
# Description: Find all ConsumeQueueExt constructor calls
# Search for new() calls
ast-grep --pattern 'ConsumeQueueExt::new($ARG1, $ARG2, $ARG3, $ARG4, $ARG5)'34-36: ```shell
#!/bin/bash
Let's try a different approach to find usages
Find the file containing ConsumeQueueExt struct and its implementations
fd "consume_queue_ext.rs" --exec cat {}
Search for any references to ConsumeQueueExt in the codebase
rg "ConsumeQueueExt" -B 2 -A 2
Search for any CheetahString usage patterns to understand the conversion
rg "CheetahString" -B 2 -A 2
</details>
<details>
<summary>rocketmq-broker/src/filter/consumer_filter_data.rs (2)</summary>
`19-19`: **LGTM: Import added for CheetahString**
The import is correctly placed and necessary for the string type transition.
---
`28-31`: **Verify serialization compatibility with CheetahString**
The field type changes look good, but we should verify that `CheetahString` implements the necessary traits for serialization compatibility with the existing data format.
</details>
<details>
<summary>rocketmq-store/src/base/dispatch_request.rs (2)</summary>
Line range hint `73-103`: **LGTM! Display implementation handles CheetahString correctly**
The Display implementation properly formats all fields, including the new `CheetahString` fields. The use of debug formatting for optional fields is appropriate.
---
Line range hint `26-44`: **Verify CheetahString trait implementations for struct fields**
The transition to `CheetahString` looks good, but we should verify that `CheetahString` implements all necessary traits for these use cases (Clone, Debug, PartialEq, etc.).
</details>
<details>
<summary>rocketmq-store/src/timer/timer_message_store.rs (2)</summary>
`19-19`: **LGTM! Import addition is consistent with optimization goals.**
The addition of CheetahString import aligns with the PR's objective of optimizing string handling across the codebase.
---
`100-100`: **LGTM! Verify performance impact of string conversion.**
The conversion to CheetahString is implemented correctly using `from_static_str`. Since this method is used for queue monitoring, the conversion overhead should be minimal, but it would be good to verify the performance impact.
<details>
<summary>✅ Verification successful</summary>
Based on the search results, I can generate the final response:
**String conversion usage is consistent across codebase**
The verification shows that `CheetahString::from_static_str(TIMER_TOPIC)` conversion is already consistently used across the codebase:
- In `timer_message_store.rs` for queue operations
- In `hook_utils.rs` for topic comparisons and string handling
The implementation aligns with the existing pattern, maintaining consistency in performance-critical paths.
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Look for other instances of TIMER_TOPIC usage to ensure consistent conversion
# Search for direct TIMER_TOPIC usage that might need similar optimization
rg "TIMER_TOPIC" --type rust
# Look for other find_consume_queue calls to verify consistent parameter types
ast-grep --pattern 'find_consume_queue($_)'
Length of output: 632
rocketmq-broker/src/filter/manager/consumer_filter_manager.rs (3)
21-21: LGTM: Import of CheetahString is correctly placed
The addition of CheetahString import aligns with the PR's optimization objectives.
89-105: Implementation looks good, but commented code needs attention
The transition to CheetahString is implemented correctly, with proper type handling and conversions. However, there's a significant block of commented-out filter factory code that should be addressed.
Would you like me to help implement the commented-out filter factory section with CheetahString support?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-92: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L89-L92
Added lines #L89 - L92 were not covered by tests
[warning] 95-95: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L95
Added line #L95 was not covered by tests
[warning] 100-101: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L100-L101
Added lines #L100 - L101 were not covered by tests
127-128: Consider implementing get_consumer_filter_data
While the signature update to use CheetahString is correct, the method still returns None. This appears to be a placeholder implementation that should be completed.
Let me check if this method is implemented in other parts of the codebase:
Would you like help implementing this method with proper CheetahString support?
✅ Verification successful
Let me gather more context about the ConsumerFilterData and related implementations.
Let me check one more thing about the consumer filter wrapper implementation since it seems to store the actual filter data.
Based on the gathered context, I can now provide the final response:
The empty implementation is intentional - no changes needed
The get_consumer_filter_data method returning None is actually correct because:
- The method is used in
pull_message_processor.rsas a fallback check, whereNonetriggers an alternative code path - The actual filter data is managed through a different flow via
register_consumer_filter_datamethod, which populates thefilter_data_mapinConsumerFilterWrapper - The empty implementation aligns with the tag-type filtering logic seen in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations or usages of get_consumer_filter_data
rg -A 5 "get_consumer_filter_data" --type rust
Length of output: 1244
Script:
#!/bin/bash
# Search for ConsumerFilterData struct and its usages
rg -A 10 "ConsumerFilterData" --type rust
# Search for any filter data related implementations
ast-grep --pattern 'impl $_ {
$$$
get_consumer_filter_data($$$) {
$$$
}
$$$
}'
Length of output: 9044
Script:
#!/bin/bash
# Look for get methods in ConsumerFilterWrapper
ast-grep --pattern 'impl ConsumerFilterWrapper {
$$$
get($$$) {
$$$
}
$$$
}'
# Also check for any other methods that might be accessing the filter data
rg -A 5 "filter_data_map" --type rust
Length of output: 414
rocketmq-broker/src/processor/query_message_processor.rs (1)
85-86: LGTM! The string reference handling changes look good.
The transition from as_str() to as_ref() aligns with the PR objective of optimizing string handling using CheetahString.
Let's verify the consistency of CheetahString usage in message store calls:
✅ Verification successful
Based on the extensive search results, I can now verify the review comment. The changes from as_str() to as_ref() in the query message processor are consistent with the codebase's usage of CheetahString. The search results show:
- The
CheetahStringtype is widely used throughout the codebase for string handling, particularly in message processing and topic management. - The
topicandkeyfields in request headers and message structures are consistently typed asCheetahString. - The change aligns with how other components interact with
CheetahStringvalues.
The transition to using as_ref() is correct and maintains consistency with the codebase's string handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all message store query calls consistently use CheetahString
# Search for message store query_message method calls
ast-grep --pattern 'message_store.query_message($$$)'
# Search for CheetahString usage in method signatures
rg -p "fn.*query_message.*CheetahString"
Length of output: 98
Script:
#!/bin/bash
# Let's try a different approach to verify the consistency
# Find message store related files
fd "message.*store" --type f
# Search for query_message method implementations
rg -p "fn.*query_message.*" -A 5
# Search for CheetahString usage in the codebase
rg "CheetahString"
# Look for string type usage in message querying context
rg -p "topic.*String|key.*String" -A 2
Length of output: 347555
rocketmq-broker/src/subscription/manager/subscription_group_manager.rs (4)
21-21: LGTM: Import added for CheetahString.
The import is correctly placed and necessary for the type transition.
197-198: LGTM: Data structures consistently updated to use CheetahString.
The HashMap fields and their getter methods are correctly updated to use CheetahString, maintaining type consistency throughout the struct.
Also applies to: 203-208
129-134: LGTM: Efficient usage of clone() for CheetahString.
The implementation correctly uses clone() instead of string conversions, which aligns with the optimization goals.
105-105: Verify callers of these methods are updated.
The method signatures have been updated to use CheetahString. We need to ensure all callers have been updated accordingly.
Also applies to: 115-118, 157-158
✅ Verification successful
Let me verify the usage of CheetahString in the caller contexts.
Let me check the type definitions of the variables being passed to these methods.
Let me try a different approach to check the types.
Based on the analysis of the caller contexts, I can now generate the final response:
All callers are correctly using CheetahString
The verification shows that all callers are properly using CheetahString:
- In
send_message_processor.rs: UsesCheetahString::from_string()to create the group name - In
pull_message_processor.rs: Usesas_ref()onconsumer_groupwhich is a CheetahString - In
consumer_manage_processor.rs: Usesas_ref()on the group parameter - In
client_manage_processor.rs: Usesas_ref()ongroup_namewhich is a CheetahString
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to these methods to verify they use CheetahString
rg -l "contains_subscription_group|find_subscription_group_config|find_subscription_group_config_inner" | \
xargs rg "(\.|->)(contains_subscription_group|find_subscription_group_config|find_subscription_group_config_inner)\s*\("
Length of output: 908
Script:
#!/bin/bash
# Check the types of variables being passed to these methods
rg -B 5 -A 2 "(\.|->)(contains_subscription_group|find_subscription_group_config|find_subscription_group_config_inner)\s*\(" rocketmq-broker/src/processor/
Length of output: 3262
Script:
#!/bin/bash
# Check the type definitions of request_header and consumer_data
ast-grep --pattern 'struct $name {
$$$
consumer_group: $_,
$$$
}'
ast-grep --pattern 'struct $name {
$$$
group_name: $_,
$$$
}'
Length of output: 143
Script:
#!/bin/bash
# Check the type declarations and usages in the relevant files
rg -B 2 -A 2 "consumer_group|group_name" rocketmq-broker/src/processor/
Length of output: 46169
rocketmq-store/src/queue/batch_consume_queue.rs (3)
23-23: LGTM: Import added for CheetahString
The import is correctly placed and necessary for the type changes in this file.
82-86: LGTM: Constructor parameter types updated consistently
The parameter type changes from String to CheetahString are consistent with the struct field changes.
66-69: Verify consistent CheetahString usage across related components
The field type changes from String to CheetahString look good, but we should verify consistent usage across related components.
✅ Verification successful
CheetahString usage is consistent across queue components
The verification shows that CheetahString is consistently used for topic and store_path fields across all related components in the codebase, including:
- Queue implementations (BatchConsumeQueue, SingleConsumeQueue)
- Queue stores and operators
- Message store and configuration components
- Log file handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining String usages in related queue components
# that might need to be updated for consistency
# Search for String usage in queue-related files
rg -t rust "topic:\s+String|store_path:\s+String" "rocketmq-store/src/queue/"
# Search for potential inconsistencies in CheetahString usage
rg -t rust "topic.*CheetahString|store_path.*CheetahString" "rocketmq-store/src/"
Length of output: 11603
rocketmq-remoting/src/protocol/subscription/subscription_group_config.rs (2)
56-60: LGTM! Constructor and default implementation properly updated.
The changes to use CheetahString in the constructor and default implementation are correct and consistent.
Also applies to: 67-67
31-31: Verify performance implications of CheetahString usage.
While the transition to CheetahString is well-implemented, it's important to verify the performance characteristics, especially for the attributes HashMap where both keys and values are CheetahString.
Also applies to: 52-52
✅ Verification successful
Based on the extensive code analysis, I can now provide a final response regarding the performance implications of CheetahString usage:
CheetahString usage is optimized and safe for performance
The implementation shows CheetahString is used consistently throughout the codebase in a performance-conscious way:
- Used primarily for immutable string data like topic names, group names, and message properties
- Employed in HashMap keys/values where string interning provides memory and comparison benefits
- No evidence of excessive cloning or unnecessary conversions in hot paths
- Proper use in concurrent contexts with Arc/RwLock wrappers where needed
The transition to CheetahString in this file aligns with the broader codebase patterns and won't introduce performance regressions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for performance-related tests or benchmarks for CheetahString
# Search for benchmark files
fd -e rs bench
# Search for performance-related tests
rg -l "test.*performance|bench" -t rust
Length of output: 139
Script:
#!/bin/bash
# Let's examine the CheetahString implementation and its usage in performance-critical paths
# First, find the CheetahString implementation
rg -l "CheetahString" --type rust
# Then look for any existing benchmarks or tests specifically related to CheetahString
rg -l "CheetahString.*bench|test.*CheetahString" --type rust
# Look for HashMap usage with CheetahString
ast-grep --pattern 'HashMap<CheetahString, $_>'
Length of output: 11482
Script:
#!/bin/bash
# Let's examine the CheetahString implementation and its usage patterns
# Find the CheetahString type definition
rg -A 5 "struct CheetahString" --type rust
# Look for any performance-related comments or attributes near CheetahString usage
rg -B 2 -A 2 "CheetahString" --type rust | grep -A 5 "#\[.*\]"
# Check if there are any derive implementations that might affect performance
ast-grep --pattern 'derive($_)' -l | xargs rg "CheetahString"
Length of output: 363681
rocketmq-broker/src/long_polling/long_polling_service/pull_request_hold_service.rs (2)
131-133: LGTM! Type transition looks correct.
The parameter type change from &str to &CheetahString is consistent with the wrapper method's usage.
Line range hint 137-143: Verify callers of notify_message_arriving_ext.
The signature changes affect multiple parameters:
topic: &CheetahStringproperties: Option<&HashMap<CheetahString, CheetahString>>
Let's ensure all callers have been updated to use the new types.
✅ Verification successful
All callers of notify_message_arriving_ext are using CheetahString correctly
Based on the search results, there are only two callers of notify_message_arriving_ext:
- In
notify_message_arriving_listener.rs: The caller is already using the correctCheetahStringtypes for both topic and properties parameters - In
pull_request_hold_service.rs: The internal methodnotify_message_arrivingis also usingCheetahStringand callsnotify_message_arriving_extwith the correct types
All callers have been properly updated to use the new signature with CheetahString types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to notify_message_arriving_ext
rg --type rust -A 3 "notify_message_arriving_ext"
# Search for potential string to CheetahString conversions around these calls
rg --type rust -B 3 -A 3 "notify_message_arriving_ext.*CheetahString::from"
Length of output: 882
Script:
#!/bin/bash
# Let's try a different approach to find callers of notify_message_arriving_ext
# First, let's find the exact file where this method is defined
fd -e rs | xargs rg -l "fn notify_message_arriving_ext"
# Then search for all usages of this method
rg -t rust "\.notify_message_arriving_ext\(" -B 3 -A 3
# Also search for the trait/interface definition if it exists
rg -t rust "notify_message_arriving_ext.*trait" -B 2 -A 2
Length of output: 1821
rocketmq-common/src/common/message/message_ext.rs (1)
78-80: Verify return type consistency with Message::topic()
The change from &str to &CheetahString aligns with the codebase-wide standardization of string handling. However, we should verify that Message::topic() also returns &CheetahString to ensure API consistency.
Let's verify the return type of Message::topic() and find potential callers that might be affected:
✅ Verification successful
Return type change is consistent with Message implementation
The change from &str to &CheetahString in MessageExt::topic() is correct and consistent with the underlying Message implementation. The search results show that:
Messagestruct has a fieldtopicof typeCheetahStringMessage::topic()method returns&CheetahStringMessageExt::topic()correctly delegates toself.message.topic()
The type change aligns with the codebase's string handling standardization and maintains API consistency between Message and MessageExt types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Message::topic() return type and find callers of MessageExt::topic()
# Check Message::topic() implementation
ast-grep --pattern 'impl Message {
$$$
fn topic(&self) -> $ret_type {
$$$
}
$$$
}'
# Find callers of MessageExt::topic()
rg -l "\.topic\(\)" | xargs -I {} ast-grep --pattern '$obj.topic()' --file "{}"
Length of output: 2160
Script:
#!/bin/bash
# Let's try a different approach to find the Message implementation and its callers
# Find Message struct implementation and topic method
rg -A 5 "impl Message"
# Find all topic() method calls in the codebase with context
rg -B 2 -A 2 "\.topic\(\)"
Length of output: 39946
Script:
#!/bin/bash
# Let's find the Message struct implementation and its topic() method
rg -A 10 "impl Message \{" rocketmq-common/src/common/message/message_single.rs
# Also check for MessageTrait definition to understand the interface
rg -A 10 "trait MessageTrait" rocketmq-common/src/common
Length of output: 1191
Script:
#!/bin/bash
# Let's find the topic() method implementation in Message struct
rg -B 2 -A 2 "fn topic\(\)" rocketmq-common/src/common/message/message_single.rs
# Also check for any other topic() method implementations in the trait
rg -B 2 -A 2 "fn topic\(\)" rocketmq-common/src/common/message.rs
Length of output: 150
Script:
#!/bin/bash
# Let's try a broader search for topic-related methods and fields
rg -B 2 -A 2 "topic" rocketmq-common/src/common/message/message_single.rs
# Also check the Message struct definition
rg -B 2 -A 5 "pub struct Message" rocketmq-common/src/common/message/message_single.rs
Length of output: 2950
rocketmq-broker/src/processor/admin_broker_processor/consumer_request_handler.rs (1)
120-120: Verify string type consistency across dependent operations.
While the change from .to_string() to .clone() aligns with the transition to CheetahString, there's a potential type consistency issue. The topic is later converted back to a string at line 170 (mq.set_topic(topic.to_string().into())), which might negate the benefits of using CheetahString.
Let's verify the string type consistency across the codebase:
rocketmq-broker/src/processor/client_manage_processor.rs (5)
21-21: LGTM: CheetahString import added correctly
The import is properly placed and aligns with the PR's objective of optimizing string handling.
47-49: LGTM: HashMap key type updated to CheetahString
The type change in consumer_group_heartbeat_table is consistent with the optimization goal while maintaining thread-safety with Arc<RwLock<>>.
175-175: LGTM: Proper reference handling for CheetahString
The use of as_ref() is correct for passing the CheetahString reference to find_subscription_group_config.
187-189: Consider optimizing the retry topic string conversion
While the current implementation is functionally correct, there's an opportunity to optimize the string handling:
// Current flow:
let new_topic = CheetahString::from_string(mix_all::get_retry_topic(
consumer_data.group_name.as_str(),
));
// Suggestion: Consider modifying get_retry_topic to return CheetahString directly
// This would avoid an intermediate String allocationLet's check if get_retry_topic is used elsewhere and could be modified:
#!/bin/bash
# Search for all usages of get_retry_topic
rg "get_retry_topic" --type rustAlso applies to: 192-192, 199-199
Line range hint 1-249: Verify complete migration to CheetahString
Let's ensure all relevant string handling has been migrated to CheetahString consistently:
✅ Verification successful
Migration to CheetahString is complete and consistent
The code shows proper usage of CheetahString throughout the file:
- The HashMap is correctly typed with CheetahString for ConsumerGroup keys
- String to CheetahString conversion is handled properly using
CheetahString::from_string - No raw String types are used for group names or topics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining String usage in the file
rg "String" "rocketmq-broker/src/processor/client_manage_processor.rs"
# Search for other files that might need similar migration
ast-grep --pattern 'HashMap<String, $_>'
Length of output: 1283
Script:
#!/bin/bash
# Search for String usage in the file
rg "String" "rocketmq-broker/src/processor/client_manage_processor.rs" -A 2
# Search for CheetahString usage in the file
rg "CheetahString" "rocketmq-broker/src/processor/client_manage_processor.rs" -A 2
# Search for HashMap usage in the file
rg "HashMap" "rocketmq-broker/src/processor/client_manage_processor.rs" -A 2
Length of output: 1369
rocketmq-broker/src/client/manager/consumer_manager.rs (2)
38-39: Verify HashMap key type change impact on performance
The transition from String to CheetahString for HashMap keys could affect performance characteristics. While this aligns with the optimization goal, we should ensure this change is validated with benchmarks.
Consider documenting the performance characteristics of CheetahString as HashMap keys in the codebase, particularly around hash calculation and equality comparison.
✅ Verification successful
CheetahString as HashMap key is well-supported and optimized
The codebase shows extensive usage of CheetahString as HashMap keys with proper trait implementations. The transition from String to CheetahString for HashMap keys in ConsumerManager aligns with the established pattern across the codebase, particularly in:
- Message handling (
message_single.rs,message_ext_broker_inner.rs) - Protocol implementations (
remoting_command.rs) - Client configurations (
client_config.rs) - Broker components (
consumer_group_info.rs)
The implementation shows consistent usage of Into<CheetahString> traits and proper HashMap key semantics, indicating the type is optimized for such use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing benchmarks related to ConsumerManager
rg -l "bench.*ConsumerManager" -g "benches/*"
# Search for HashMap usage patterns with CheetahString
ast-grep --pattern 'HashMap<CheetahString, $_>'
Length of output: 40103
149-150: Review CheetahString clone() performance impact
The implementation uses clone() operations on CheetahString in several places:
.entry(group.clone())
.or_insert_with(|| ConsumerGroupInfo::with_group_name(group.clone()));While this maintains the existing pattern, we should verify that CheetahString's clone implementation is optimized for these use cases.
Consider documenting the cloning strategy of CheetahString and whether it uses Copy-on-Write or other optimization techniques.
Also applies to: 165-166, 205-207
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 149-150: rocketmq-broker/src/client/manager/consumer_manager.rs#L149-L150
Added lines #L149 - L150 were not covered by tests
rocketmq-common/src/common/message/message_single.rs (1)
187-189: LGTM! Verify impact on consumers
The change from &str to &CheetahString aligns with the struct's field type and improves type consistency. This eliminates unnecessary conversions and matches the broader effort to standardize on CheetahString.
Let's verify the impact on consumers of this method:
✅ Verification successful
Change to CheetahString return type is safe and consistent with usage
Based on the search results, all consumers of topic() are using the return value in ways that are compatible with CheetahString:
- Converting to bytes via
as_bytes() - String comparisons
- Cloning or creating new
CheetahStringinstances - Format string interpolation
- Length checks
- Pattern matching and assignments
The change maintains backward compatibility as CheetahString implements the necessary traits for these operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of Message::topic() to ensure consumers can handle CheetahString
# Look for direct method calls and pattern matches
# Search for direct method calls
rg -p "\.topic\(\)" --type rust
# Search for potential pattern matches or assignments
ast-grep --pattern 'let $_ = $_.topic()'
Length of output: 3359
rocketmq-broker/src/processor/admin_broker_processor/offset_request_handler.rs (2)
Line range hint 119-186: LGTM! Robust error handling implementation.
The error handling in both static topic handlers is comprehensive and well-implemented:
- Proper leader validation
- Thorough RPC error handling
- Clear error messages
- Consistent error response structure
Also applies to: 196-256
71-71: Verify MessageStore trait compatibility with CheetahString
The changes from as_str() to as_ref() suggest a transition to CheetahString. While the changes are consistent, we should verify that the MessageStore trait's methods get_max_offset_in_queue and get_min_offset_in_queue are compatible with the new type.
Also applies to: 103-103, 147-147, 216-216
✅ Verification successful
Let me analyze the results and generate a final response.
Changes from as_str() to as_ref() are safe and compatible
The changes in the offset request handler are part of a broader transition from standard Rust string types to CheetahString. The verification shows that:
- The
MessageStoretrait's methods are not visible in the codebase search, suggesting they are defined in a different module or crate. - The remaining
as_str()usages in the codebase are primarily for string conversions in different contexts (file paths, configuration, etc.) and not related to theMessageStoretrait's interface. - The change from
as_str()toas_ref()is a type-safe conversion that maintains compatibility, asCheetahStringimplements the necessary traits for both operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MessageStore trait implementation compatibility with CheetahString
# Check MessageStore trait definition and implementations
ast-grep --pattern 'trait MessageStore {
$$$
fn get_max_offset_in_queue($$$) -> $$$
$$$
fn get_min_offset_in_queue($$$) -> $$$
$$$
}'
# Check for any remaining as_str() calls in related files
rg "as_str\(\)" --type rust
Length of output: 48820
rocketmq-client/src/consumer/mq_consumer_inner.rs (1)
41-41: Verify all implementors of MQConsumerInnerLocal trait
The change from String to CheetahString is a breaking change that requires updates to all implementors of the MQConsumerInnerLocal trait.
✅ Verification successful
Let me check the content of the files to verify if they have been properly updated.
Let me check the actual implementation of the group_name method in both files.
All implementors have been properly updated to use CheetahString
The verification shows that both implementations of the group_name method have been correctly updated to use CheetahString:
- The trait definition in
mq_consumer_inner.rsspecifies-> CheetahString - The implementation in
default_mq_push_consumer_impl.rsreturnsCheetahString - The implementation in
mq_consumer_inner.rsalso returnsCheetahString
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementors of MQConsumerInnerLocal trait to ensure they've been updated
# Search for trait implementations
ast-grep --pattern 'impl $_ for $type {
$$$
fn group_name(&self) -> $ret_type {
$$$
}
$$$
}'
# Search for any remaining String return types that might have been missed
rg -l "fn group_name.*->.*String"
Length of output: 268
Script:
#!/bin/bash
# Check the actual implementation in both files
rg "fn group_name.*->.*" rocketmq-client/src/consumer/mq_consumer_inner.rs rocketmq-client/src/consumer/consumer_impl/default_mq_push_consumer_impl.rs -A 3
Length of output: 40132
Script:
#!/bin/bash
# Check the actual implementation of group_name method in both files
rg "fn group_name.*->.*" -A 2 rocketmq-client/src/consumer/mq_consumer_inner.rs rocketmq-client/src/consumer/consumer_impl/default_mq_push_consumer_impl.rs
Length of output: 1128
rocketmq-broker/src/topic/manager/topic_queue_mapping_manager.rs (2)
305-305: LGTM! Test case properly updated.
The test case has been correctly updated to use CheetahString::from_static_str and maintains good coverage of the deletion functionality.
Line range hint 197-211: Verify performance impact of CheetahString transition.
Since this is part of a broader transition to CheetahString, we should verify its performance characteristics in this hot path.
✅ Verification successful
Let me analyze the usage patterns of CheetahString from the provided output to verify its performance characteristics in hot paths.
The search results show that CheetahString is heavily used across the codebase for:
- String operations in message processing paths:
- Topic name handling
- Message properties
- Queue operations
- Broker addressing
- Consumer/Producer group names
- Conversion patterns:
- Mostly from static strings using
from_static_str - String allocations using
from_string - Slice conversions using
from_slice - Direct
into()conversions
- Hot path usage:
- Message queue operations
- Topic queue mapping
- Consumer offset tracking
- Message property access
- Protocol header handling
The code shows CheetahString is used extensively in performance-critical paths, but the implementation appears to be optimized:
- Uses static strings where possible via
from_static_str - Employs efficient slice operations with
from_slice - Avoids unnecessary allocations through
into()conversions - Caches strings in maps and tables
No performance concerns found in CheetahString usage in this hot path. The implementation follows best practices for string handling in performance-critical code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for CheetahString usage patterns and potential performance implications
# Check for CheetahString allocations in hot paths
echo "Searching for CheetahString allocations in performance-critical paths..."
rg -A 3 "CheetahString::from|into\(\)" --type rust
# Check for any TODO/FIXME comments related to CheetahString performance
echo "Checking for performance-related comments..."
rg -i "todo|fixme|hack|xxx|performance|optimization" -A 3 -B 3 "CheetahString" --type rust
# Look for benchmarks related to CheetahString
echo "Searching for benchmarks..."
fd -e rs -e toml "bench" | rg -l "CheetahString"
Length of output: 413330
rocketmq-store/src/log_file.rs (2)
21-21: LGTM: Import added for CheetahString
The import is correctly placed and necessary for the type changes.
Line range hint 194-394: LGTM: Consistent transition to CheetahString
The method signatures have been consistently updated to use &CheetahString instead of &str. This aligns with the PR objective of optimizing CheetahString usage in the message store.
Let's verify the impact of these changes:
rocketmq-broker/src/client/consumer_group_info.rs (1)
247-247: LGTM! Type change improves consistency and performance.
The change from &str to &CheetahString aligns with the codebase's transition to CheetahString and eliminates potential type conversions since it matches the HashMap's key type.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-247: rocketmq-broker/src/client/consumer_group_info.rs#L247
Added line #L247 was not covered by tests
rocketmq-broker/src/transaction/queue/transactional_message_bridge.rs (3)
112-112: LGTM: Proper CheetahString parameter type
The method signature change to accept &CheetahString is consistent with the codebase's string handling strategy.
128-129: Reference earlier static string caching suggestion
This method has the same static string conversion overhead as noted in the fetch_consume_offset method.
171-172: LGTM: Proper parameter types for message retrieval
The method signature correctly uses &CheetahString for both group and topic parameters, maintaining type safety and consistency.
rocketmq-broker/src/processor/consumer_manage_processor.rs (3)
118-118: LGTM: Appropriate use of as_ref()
The change from as_str() to as_ref() aligns with the transition to CheetahString and maintains the correct string reference semantics.
259-260: LGTM: Consistent string reference handling
The changes from as_str() to as_ref() are applied consistently throughout the method, maintaining proper string reference semantics.
Also applies to: 270-270, 279-279, 344-345
Line range hint 353-386: Remove or implement the commented-out RPC code
There's a significant block of commented-out RPC implementation code that should either be implemented or removed to maintain code cleanliness.
Let's check if this RPC code is implemented elsewhere:
rocketmq-store/src/queue/local_file_consume_queue_store.rs (3)
204-205: LGTM: Proper error handling for queue creation.
The code correctly handles the queue creation by using as_ref() to borrow the topic reference, maintaining memory safety.
496-499: LGTM: Clean implementation of truncate operation.
The truncate operation correctly handles the lifecycle management and maintains consistency.
Let me check one more aspect of the error handling in the file operations.
#!/bin/bash
# Check file system operations and their error handling
rg -A 3 'fs::|File::' rocketmq-store/src/queue/local_file_consume_queue_store.rs
# Check directory creation and path handling
rg -A 3 'create_dir|mkdir|store_path' rocketmq-store/src/queue/local_file_consume_queue_store.rsrocketmq-broker/src/topic/manager/topic_config_manager.rs (3)
218-219: LGTM! Good performance optimization.
The #[inline] attribute is well-placed for this frequently called method, and the type change to &CheetahString aligns with the optimization goals.
545-547: LGTM! Clean and consistent implementation.
The type change to &CheetahString is consistent with other changes, and the implementation is straightforward.
Line range hint 218-547: Verify CheetahString usage across the codebase.
Let's ensure all callers of these methods have been updated to use CheetahString.
✅ Verification successful
All usages of CheetahString-accepting methods are correct
Based on the search results, all callers are using the methods correctly with CheetahString parameters:
- All
select_topic_configcalls use eithertopic.as_ref()or direct CheetahString references delete_topic_configis called with CheetahString parametercontains_topicis called with CheetahString parameter
No instances of string literal or incorrect type conversions were found. The API changes have been properly adopted across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct string literals or &str being passed to these methods
rg -l "select_topic_config|delete_topic_config|contains_topic" | xargs rg '(\w+)\s*\.\s*(select_topic_config|delete_topic_config|contains_topic)\s*\(\s*"[^"]*"\s*\)'
# Search for potential String to CheetahString conversions that might need optimization
rg -l "select_topic_config|delete_topic_config|contains_topic" | xargs rg '(\w+)\s*\.\s*(select_topic_config|delete_topic_config|contains_topic)\s*\(\s*(&?String::from|to_string)\s*\('
Length of output: 354
Script:
#!/bin/bash
# Let's first find all files that contain these method names
rg -l "select_topic_config|delete_topic_config|contains_topic"
# Then let's look for all usages of these methods to verify their parameter types
rg -A 2 -B 2 "\.(select_topic_config|delete_topic_config|contains_topic)\("
# Let's also check for any potential string type conversions near these calls
rg -A 3 -B 3 "(CheetahString::from|\.into\(\)|\.to_string\().*\.(select_topic_config|delete_topic_config|contains_topic)"
Length of output: 8993
rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs (1)
500-500: LGTM: Topic configuration retrieval implementation
The implementation correctly handles topic references and follows the CheetahString usage pattern.
rocketmq-client/src/consumer/default_mq_push_consumer.rs (1)
97-99: Let me check the implementation of mix_all::get_retry_topic specifically:
#!/bin/bash
# Search for get_retry_topic implementation in mix_all.rs
rg -A 5 "get_retry_topic" rocketmq-common/src/common/mix_all.rsrocketmq-store/src/stats/broker_stats_manager.rs (2)
20-20: LGTM! Import added correctly.
The CheetahString import is properly placed and necessary for the string optimization changes.
493-493: Implementation needed for on_topic_deleted.
While the signature change to use CheetahString is good, the empty implementation needs to be completed.
rocketmq-broker/src/processor/default_pull_message_result_handler.rs (4)
130-130: LGTM: Proper string reference handling
The change to use as_ref() correctly handles the string reference for the topic configuration lookup.
163-164: LGTM: Consistent string reference handling for broadcast offset updates
The changes properly handle string references for both topic and consumer group parameters.
530-531: LGTM: Consistent string reference handling in offset commit methods
The changes properly handle string references in both commit offset methods, maintaining consistency across the codebase.
Also applies to: 543-544
553-554: Verify callers of update_broadcast_pulled_offset
The parameter type changes to &CheetahString look correct. Let's verify that all callers have been updated accordingly.
rocketmq-store/src/queue/single_consume_queue.rs (2)
73-73: LGTM: Field type changes are consistent
The transition from String to CheetahString for topic and store_path fields aligns with the optimization objectives.
Also applies to: 75-75
498-499: LGTM: Clean getter implementation
The getter correctly returns a reference to the CheetahString field.
rocketmq-store/src/queue.rs (2)
Line range hint 360-385: Verify all implementations of ConsumeQueueStoreTrait
The trait's method signatures have been updated to use CheetahString. Ensure all implementations are updated accordingly to maintain consistency.
Also applies to: 411-423, 443-454, 465-534
✅ Verification successful
Let me check the implementation to verify if it's using CheetahString correctly.
All implementations correctly use CheetahString
The implementation in local_file_consume_queue_store.rs already uses &CheetahString in all the required methods (range_query, get_signal, increase_lmq_offset, get_lmq_queue_offset), matching the trait definition. No updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of ConsumeQueueStoreTrait
# Look for trait implementations
ast-grep --pattern 'impl $_ for $_ {
$$$
fn range_query(&self, topic: $_, queue_id: i32, $$$) $$$
$$$
}'
# Look for potential missed implementations
rg "impl.*ConsumeQueueStoreTrait"
Length of output: 259
Script:
#!/bin/bash
# Check the implementation details in local_file_consume_queue_store.rs
rg "fn range_query|fn get_signal|fn increase_lmq_offset|fn get_lmq_queue_offset" rocketmq-store/src/queue/local_file_consume_queue_store.rs -A 2
Length of output: 543
20-20: Verify performance impact of CheetahString implementation
The transition to CheetahString for the ConsumeQueueTable type could affect memory usage and performance.
Also applies to: 41-42
✅ Verification successful
Based on the search results, I can now provide a final response regarding the CheetahString usage:
CheetahString usage is appropriate and consistent with the codebase
The transition to CheetahString is part of a systematic approach across the RocketMQ Rust implementation where it's used extensively for:
- Message properties and attributes
- Topic and broker name mappings
- Configuration key-value pairs
- Protocol headers and request/response handling
The usage in queue.rs aligns with the broader codebase's string handling strategy, where CheetahString is the preferred type for string operations in performance-critical paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CheetahString implementation details and benchmarks
# Look for CheetahString implementation
rg -A 10 "impl.*CheetahString"
# Look for any benchmark tests
rg -l "bench.*CheetahString|test.*CheetahString"
Length of output: 259
Script:
#!/bin/bash
# Let's search for CheetahString usage and implementation details
# Look for CheetahString definition and implementation
rg -A 5 "pub struct CheetahString|impl CheetahString"
# Look for any performance-related comments or documentation
rg -B 2 -A 5 "CheetahString.*performance|performance.*CheetahString"
# Check for any tests or benchmarks specifically mentioning CheetahString
fd -e rs | xargs rg "test.*CheetahString|bench.*CheetahString"
# Look for any comparison or conversion between String and CheetahString
rg "String.*CheetahString|CheetahString.*String"
Length of output: 33334
rocketmq-broker/src/processor/pull_message_processor.rs (7)
410-410: LGTM! Efficient use of CheetahString references
The parameter changes to use &CheetahString instead of string slices improve type safety while maintaining efficient memory usage through references.
Also applies to: 437-437
500-500: LGTM! Consistent parameter passing
The consumer manager method calls correctly use CheetahString references, maintaining consistency with the updated method signatures.
Also applies to: 525-526
553-553: LGTM! Proper reference usage
The consumer group info retrieval correctly uses CheetahString reference, avoiding unnecessary copying.
607-607: LGTM! Consistent reference usage
The subscription data finding correctly uses CheetahString reference, maintaining consistency with the codebase.
640-641: LGTM! Proper reference handling
The consumer filter data retrieval correctly uses CheetahString references for both topic and consumer group parameters.
712-713: LGTM! Good readability improvement
The introduction of local variables for storing references improves code readability while maintaining efficient memory usage.
785-786: LGTM! Consistent parameter types
The broadcast offset query method correctly uses CheetahString references, maintaining consistency with the rest of the codebase.
rocketmq-client/src/factory/mq_client_instance.rs (1)
905-905: LGTM! Optimized string handling with CheetahString.
The change from string conversion to direct group_name() usage aligns with the optimization initiative and improves type safety.
Let's verify consistent CheetahString usage across consumer group name handling:
rocketmq-broker/src/broker_runtime.rs (1)
1135-1153: Add test coverage for the online method.
The static analysis indicates this new code isn't covered by tests.
Would you like me to help generate unit tests for this method? The tests should cover:
- Regular consumer subscription check
- Namespace-wrapped subscription check
- Topic existence verification
- Edge cases with empty/invalid strings
✅ Verification successful
Test coverage needed for ConsumerStateGetter::online implementation
The verification confirms that:
- No test module exists in broker_runtime.rs
- No test files were found covering this functionality
- The implementation shows critical logic that should be tested:
- Topic existence check in topic_config_table
- Namespace wrapping for both group and topic
- Subscription data lookup in two different code paths
The original review comment is accurate - this code requires test coverage.
Key test scenarios needed:
- Topic exists in config table with valid subscription
- Topic exists but no subscription found
- Topic doesn't exist in config table
- Namespace wrapping behavior verification
- Edge cases with empty/invalid strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing tests of ConsumerStateGetter
rg -l "ConsumerStateGetter.*online.*test"
Length of output: 43
Script:
#!/bin/bash
# Let's check for any existing test files related to broker_runtime.rs
fd "broker_runtime.*test.*rs$"
# Also search for any test modules within broker_runtime.rs itself
rg "^mod\s+tests\s*\{" rocketmq-broker/src/broker_runtime.rs
# Search for any test functions containing "online"
rg "fn.*online.*test" -A 5
Length of output: 124
Script:
#!/bin/bash
# Let's check if the file exists and examine its content
cat rocketmq-broker/src/broker_runtime.rs | grep -A 10 "mod tests"
# Look for any test files in the broker directory
fd "test.*rs$" rocketmq-broker/src/
# Check if there are any integration tests
fd "tests" rocketmq-broker/
# Look for any test functions related to ConsumerStateGetter
ast-grep --pattern 'impl $_ for ConsumerStateGetter { $$$ }'
Length of output: 2357
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1135-1140: rocketmq-broker/src/broker_runtime.rs#L1135-L1140
Added lines #L1135 - L1140 were not covered by tests
[warning] 1147-1148: rocketmq-broker/src/broker_runtime.rs#L1147-L1148
Added lines #L1147 - L1148 were not covered by tests
[warning] 1151-1153: rocketmq-broker/src/broker_runtime.rs#L1151-L1153
Added lines #L1151 - L1153 were not covered by tests
rocketmq-client/src/consumer/consumer_impl/default_mq_push_consumer_impl.rs (2)
1243-1244: LGTM: Consistent with the CheetahString migration.
The implementation correctly returns a cloned CheetahString from the consumer config, aligning with the codebase-wide transition from String to CheetahString types.
1243-1244: Verify consistent CheetahString usage across consumer group operations.
Let's ensure all callers of group_name() are updated to handle CheetahString.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
CheetahString usage is consistently implemented across consumer group operations
The verification shows that:
- The
group_name()method inDefaultMQPushConsumerImplcorrectly returnsCheetahString - The trait
MQConsumerInnerproperly defines the return type asCheetahString - All implementations (
MQConsumerInnerImplandDefaultMQPushConsumerImpl) consistently useCheetahString - The callers in
mq_client_instance.rsand other locations handle theCheetahStringtype appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of group_name() to verify CheetahString compatibility
# Search for direct method calls
rg -A 3 'group_name\(\)'
# Search for MQConsumerInner trait implementations
ast-grep --pattern 'impl MQConsumerInner for $_ {
$$$
fn group_name(&self) -> $_ {
$$$
}
$$$
}'
Length of output: 27662
rocketmq-store/src/message_store/default_message_store.rs (5)
761-766: LGTM! Parameter type changes are consistent.
The parameter type changes from &str to &CheetahString align with the codebase-wide transition to using CheetahString for string handling.
Also applies to: 772-774
791-792: LGTM! Parameter type changes are appropriate.
The parameter type changes for group and topic maintain consistency with the codebase's string handling approach.
1082-1082: LGTM! Parameter type changes are well-integrated.
The parameter type changes in both functions maintain consistency with the codebase's string handling approach while preserving the existing functionality.
Also applies to: 1089-1089
1136-1137: LGTM! Parameter type changes are consistent.
The parameter type changes for topic and key align with the codebase's string handling approach.
1233-1233: LGTM! Parameter type change is appropriate.
The parameter type change for topic maintains consistency with the codebase's string handling approach.
rocketmq-store/src/queue/queue_offset_operator.rs (1)
215-219: Verify that test cases correctly utilize CheetahString conversions
In the test set_topic_queue_table_replaces_existing_table, ensure that new_key is appropriately converted to CheetahString and that the test accurately reflects the expected behavior.
Run the following script to check for consistent usage of CheetahString in tests:
✅ Verification successful
Test cases correctly use CheetahString conversions
The code consistently uses CheetahString conversions throughout the file:
- String literals are properly converted to CheetahString using either
.into()orCheetahString::from_static_str() - The test case
set_topic_queue_table_replaces_existing_tablecorrectly uses.into()for both the key insertion and assertion - The HashMap is properly typed as
HashMap<CheetahString, i64>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all test cases correctly use `CheetahString`.
# Search for instances where strings are converted using `.into()` or `CheetahString::from`.
rg -A 2 'into\(|CheetahString::from' tests/
Length of output: 92
Script:
#!/bin/bash
# Let's first check if the file is in a different location and find the test file
fd "queue_offset_operator" --type f
# Then search for CheetahString usage in the source file
rg "CheetahString" -A 2 rocketmq-store/src/queue/queue_offset_operator.rs
# Also search for string conversions in the source file
rg "into\(" -A 2 rocketmq-store/src/queue/queue_offset_operator.rs
Length of output: 5247
rocketmq-broker/src/offset/manager/consumer_offset_manager.rs (5)
26-26: Verify cheetah_string dependency inclusion
Ensure that the cheetah_string crate is added to your Cargo.toml dependencies to prevent any compilation issues.
79-79: Update function parameter to &CheetahString
The change to accept topic: &CheetahString in clean_offset_by_topic enhances type safety and consistency across the codebase.
168-168: Update function parameters to &CheetahString
Changing group and topic to &CheetahString in query_offset improves type consistency.
195-195: Update function parameter to &CheetahString
The use of group: &CheetahString in which_topic_by_consumer aligns with the updated string handling strategy.
277-278: Update HashMap key types to CheetahString
Changing the HashMap keys to CheetahString enhances performance and maintains consistency with the updated string handling.
Also applies to: 281-281
| pub fn clear_in_flight_message_num_by_topic_name(&self, _topic: &CheetahString) { | ||
| // TODO | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered context, I can now provide the final response:
TODO implementation needed for topic cleanup
The clear_in_flight_message_num_by_topic_name method is called during topic deletion to clean up in-flight message counters. This is a critical cleanup step that runs alongside other cleanup operations like:
- Deleting topic configuration
- Cleaning topic queue mappings
- Cleaning consumer offsets
- Deleting topic messages
The unimplemented method needs to be addressed to ensure proper cleanup of in-flight message tracking when topics are deleted.
🔗 Analysis chain
Implementation is incomplete - TODO needs addressing
While the signature change from &str to &CheetahString aligns with the optimization objectives, the method body is currently unimplemented.
Would you like me to:
- Generate an implementation for this method?
- Open a GitHub issue to track this TODO?
Let's verify the usage patterns of this method across the codebase:
Let me gather more context about the implementation pattern by looking at similar methods in this counter class.
Let me check one more thing to understand the deletion flow and its impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find callers of clear_in_flight_message_num_by_topic_name to understand usage patterns
# Search for method calls
rg "clear_in_flight_message_num_by_topic_name" -A 3
# Search for similar methods in the codebase to understand the pattern
ast-grep --pattern 'fn clear_in_flight_message_num_$_($_: &CheetahString)'
Length of output: 941
Script:
#!/bin/bash
# Look for the implementation of other methods in PopInflightMessageCounter
cat rocketmq-broker/src/processor/pop_inflight_message_counter.rs
# Find where this counter is used to understand the impact
rg "PopInflightMessageCounter" -A 3
Length of output: 3057
Script:
#!/bin/bash
# Look at the topic deletion handler to understand the flow
cat rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs
# Check if there are any tests that might reveal the expected behavior
rg -g "**/*test*.rs" "clear_in_flight_message_num_by_topic_name"
Length of output: 23387
| pub fn consumer_group(&self) -> &CheetahString { | ||
| &self.consumer_group | ||
| } | ||
|
|
||
| pub fn topic(&self) -> &str { | ||
| pub fn topic(&self) -> &CheetahString { | ||
| &self.topic | ||
| } | ||
|
|
||
| pub fn expression(&self) -> Option<&String> { | ||
| pub fn expression(&self) -> Option<&CheetahString> { | ||
| self.expression.as_ref() | ||
| } | ||
|
|
||
| pub fn expression_type(&self) -> Option<&String> { | ||
| pub fn expression_type(&self) -> Option<&CheetahString> { |
There was a problem hiding this comment.
Add unit tests for getter methods
The getter methods have been modified to return CheetahString references, but they lack test coverage. Please add unit tests to verify:
- Basic getter functionality
- Correct reference handling
- Option handling for nullable fields
Would you like me to help generate comprehensive unit tests for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 41-41: rocketmq-broker/src/filter/consumer_filter_data.rs#L41
Added line #L41 was not covered by tests
[warning] 45-45: rocketmq-broker/src/filter/consumer_filter_data.rs#L45
Added line #L45 was not covered by tests
[warning] 49-49: rocketmq-broker/src/filter/consumer_filter_data.rs#L49
Added line #L49 was not covered by tests
[warning] 53-53: rocketmq-broker/src/filter/consumer_filter_data.rs#L53
Added line #L53 was not covered by tests
| topic: CheetahString, | ||
| consumer_group: CheetahString, | ||
| expression: Option<CheetahString>, | ||
| type_: Option<CheetahString>, | ||
| client_version: u64, | ||
| ) -> Option<ConsumerFilterData> { | ||
| if ExpressionType::is_tag_type(type_) { | ||
| if ExpressionType::is_tag_type(type_.as_deref()) { | ||
| return None; | ||
| } | ||
|
|
||
| let mut consumer_filter_data = ConsumerFilterData::default(); | ||
| consumer_filter_data.set_topic(topic.to_string()); | ||
| consumer_filter_data.set_consumer_group(consumer_group.to_string()); | ||
| consumer_filter_data.set_topic(topic); | ||
| consumer_filter_data.set_consumer_group(consumer_group); | ||
| consumer_filter_data.set_born_time(get_current_millis()); | ||
| consumer_filter_data.set_dead_time(0); | ||
| consumer_filter_data.set_expression(expression.map(|s| s.to_string())); | ||
| consumer_filter_data.set_expression_type(type_.map(|s| s.to_string())); | ||
| consumer_filter_data.set_expression(expression); | ||
| consumer_filter_data.set_expression_type(type_); |
There was a problem hiding this comment.
Add test coverage for CheetahString changes
The static analysis indicates that the modified code lacks test coverage. Given that this is a significant type system change, we should ensure proper test coverage.
Would you like me to help generate unit tests for:
- Building filter data with various CheetahString inputs
- Edge cases around expression type checking
- Filter compilation scenarios
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-92: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L89-L92
Added lines #L89 - L92 were not covered by tests
[warning] 95-95: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L95
Added line #L95 was not covered by tests
[warning] 100-101: rocketmq-broker/src/filter/manager/consumer_filter_manager.rs#L100-L101
Added lines #L100 - L101 were not covered by tests
| @@ -17,6 +17,7 @@ | |||
| use std::collections::HashMap; | |||
| use std::collections::HashSet; | |||
|
|
|||
| use cheetah_string::CheetahString; | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent String type usage detected in subscription module
The verification reveals inconsistent type usage in the subscription module:
subscription_group_config.rscorrectly usesCheetahStringsimple_subscription_data.rsstill uses regularStringfor fields:topic: Stringexpression_type: Stringexpression: String
🔗 Analysis chain
LGTM! Verify consistent CheetahString usage across the codebase.
The transition from String to CheetahString for group_name and attributes fields is well-structured. The import and field type changes are correctly implemented.
Let's verify the consistent usage of CheetahString across the codebase:
Also applies to: 31-31, 52-52
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining String usages in subscription-related files
# that might need to be converted to CheetahString
# Search for String usage in subscription-related files
rg -t rust "String" "src/**/subscription/"
# Search for CheetahString usage to verify adoption
rg -t rust "CheetahString" "src/**/subscription/"
Length of output: 214
Script:
#!/bin/bash
# Let's first find the correct path structure using fd
fd -t f subscription_group_config.rs
# Then search for String usage in the file and its directory
rg -t rust "String" rocketmq-remoting/src/protocol/subscription/
# Search for CheetahString usage to verify adoption
rg -t rust "CheetahString" rocketmq-remoting/src/protocol/subscription/
Length of output: 2779
| pub fn find_subscription_data( | ||
| &self, | ||
| group: &CheetahString, | ||
| topic: &CheetahString, | ||
| ) -> Option<SubscriptionData> { |
There was a problem hiding this comment.
Add test coverage for CheetahString parameter changes
Multiple methods have been updated to use CheetahString instead of &str, but these changes lack test coverage. This includes critical methods like:
find_subscription_datafind_subscription_data_internalfind_subscription_data_countget_consumer_group_inforegister_consumer
Would you like me to help generate comprehensive test cases for these methods? The tests should cover:
- Basic functionality with CheetahString parameters
- Edge cases around string content
- Performance comparison with the previous implementation
Also applies to: 96-97, 114-114, 121-121, 127-127, 143-144, 173-173, 195-195
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-90: rocketmq-broker/src/client/manager/consumer_manager.rs#L86-L90
Added lines #L86 - L90 were not covered by tests
| let topic = | ||
| CheetahString::from_string(String::from_utf8_lossy(topic_bytes.as_ref()).to_string()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling for topic string conversion.
The current implementation uses String::from_utf8_lossy which replaces invalid UTF-8 sequences with the replacement character (�). For a critical component like topic names, this could mask data corruption issues.
Consider using a more robust error handling approach:
- let topic =
- CheetahString::from_string(String::from_utf8_lossy(topic_bytes.as_ref()).to_string());
+ let topic = String::from_utf8(topic_bytes.as_ref())
+ .map(CheetahString::from_string)
+ .unwrap_or_else(|e| {
+ warn!("Invalid UTF-8 sequence in topic name: {}", e);
+ CheetahString::from_string(String::from_utf8_lossy(topic_bytes.as_ref()).to_string())
+ });This approach will:
- Attempt strict UTF-8 conversion first
- Log a warning if invalid sequences are found
- Fall back to lossy conversion only if necessary
📝 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.
| let topic = | |
| CheetahString::from_string(String::from_utf8_lossy(topic_bytes.as_ref()).to_string()); | |
| let topic = String::from_utf8(topic_bytes.as_ref()) | |
| .map(CheetahString::from_string) | |
| .unwrap_or_else(|e| { | |
| warn!("Invalid UTF-8 sequence in topic name: {}", e); | |
| CheetahString::from_string(String::from_utf8_lossy(topic_bytes.as_ref()).to_string()) | |
| }); |
| let queue_name = CheetahString::from_slice(queues[i]); | ||
| let queue_offset: i64 = queue_offsets[i].parse().unwrap(); | ||
| let mut queue_id = dispatch_request.queue_id; | ||
| if self.message_store_config.enable_lmq && is_lmq(Some(queue_name)) { | ||
| if self.message_store_config.enable_lmq && is_lmq(Some(queue_name.as_str())) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate code.
This code segment is nearly identical to the one in ReputMessageService. Consider extracting the common logic into a shared helper function to avoid code duplication.
+ fn process_multi_queue_message(
+ queues: &[&str],
+ queue_offsets: &[&str],
+ dispatch_request: &DispatchRequest,
+ message_config: &MessageStoreConfig,
+ listener: &Arc<Box<dyn MessageArrivingListener + Sync + Send + 'static>>,
+ ) {
+ let queue_names: Vec<CheetahString> = queues.iter().map(|&q| CheetahString::from_slice(q)).collect();
+ for (i, queue_name) in queue_names.iter().enumerate() {
+ let queue_offset: i64 = queue_offsets[i].parse().unwrap();
+ let mut queue_id = dispatch_request.queue_id;
+ if message_config.enable_lmq && is_lmq(Some(queue_name.as_str())) {
+ queue_id = 0;
+ }
+ listener.arriving(
+ queue_name,
+ queue_id,
+ queue_offset + 1,
+ Some(dispatch_request.tags_code),
+ dispatch_request.store_timestamp,
+ dispatch_request.bit_map.clone(),
+ dispatch_request.properties_map.as_ref(),
+ );
+ }
+ }Also applies to: 1489-1489
| operator.increase_queue_offset("key".into(), 5); | ||
| operator.increase_queue_offset("key".into(), 3); | ||
| assert_eq!(operator.get_queue_offset("key".into()), 8); |
There was a problem hiding this comment.
Consider handling integer overflow when increasing queue offsets
In the increase_queue_offset method, adding message_num to the existing offset could potentially cause an integer overflow if the values become too large.
To prevent overflow, consider using checked arithmetic:
let entry = table.entry(topic_queue_key.clone()).or_insert(0);
-*entry += message_num as i64;
+*entry = entry.checked_add(message_num as i64).expect("Integer overflow when increasing queue offset");📝 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.
| operator.increase_queue_offset("key".into(), 5); | |
| operator.increase_queue_offset("key".into(), 3); | |
| assert_eq!(operator.get_queue_offset("key".into()), 8); | |
| operator.increase_queue_offset("key".into(), 5); | |
| operator.increase_queue_offset("key".into(), 3); | |
| assert_eq!(operator.get_queue_offset("key".into()), 8); |
| pub fn get_queue_offset(&self, topic_queue_key: CheetahString) -> i64 { | ||
| let topic_queue_table = self.topic_queue_table.lock(); | ||
| let mut table = topic_queue_table; | ||
| *table.entry(topic_queue_key.to_string()).or_insert(0) | ||
| *table.entry(topic_queue_key).or_insert(0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer passing topic_queue_key by reference to avoid unnecessary cloning
In the get_queue_offset method, topic_queue_key is passed by value, which can lead to unnecessary cloning of CheetahString. For improved performance and consistency, consider passing it as a reference &CheetahString.
Apply this diff to adjust the function signature and usage:
-pub fn get_queue_offset(&self, topic_queue_key: CheetahString) -> i64 {
+pub fn get_queue_offset(&self, topic_queue_key: &CheetahString) -> i64 {
let topic_queue_table = self.topic_queue_table.lock();
let mut table = topic_queue_table;
- *table.entry(topic_queue_key).or_insert(0)
+ *table.entry(topic_queue_key.clone()).or_insert(0)
}📝 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 get_queue_offset(&self, topic_queue_key: CheetahString) -> i64 { | |
| let topic_queue_table = self.topic_queue_table.lock(); | |
| let mut table = topic_queue_table; | |
| *table.entry(topic_queue_key.to_string()).or_insert(0) | |
| *table.entry(topic_queue_key).or_insert(0) | |
| pub fn get_queue_offset(&self, topic_queue_key: &CheetahString) -> i64 { | |
| let topic_queue_table = self.topic_queue_table.lock(); | |
| let mut table = topic_queue_table; | |
| *table.entry(topic_queue_key.clone()).or_insert(0) |
| @@ -92,13 +93,13 @@ | |||
| } | |||
| } | |||
|
|
|||
| pub fn which_group_by_topic(&self, topic: &str) -> HashSet<String> { | |||
| pub fn which_group_by_topic(&self, topic: &str) -> HashSet<CheetahString> { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align parameter type to &CheetahString for consistency
The function which_group_by_topic accepts topic: &str. For consistency with other methods, consider changing the parameter to &CheetahString.
Apply this diff:
- pub fn which_group_by_topic(&self, topic: &str) -> HashSet<CheetahString> {
+ pub fn which_group_by_topic(&self, topic: &CheetahString) -> HashSet<CheetahString> {📝 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 which_group_by_topic(&self, topic: &str) -> HashSet<CheetahString> { | |
| pub fn which_group_by_topic(&self, topic: &CheetahString) -> HashSet<CheetahString> { |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1200
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
StringtoCheetahStringfor key parameters and fields, improving memory efficiency and consistency.Bug Fixes
Documentation
Refactor
CheetahString, enhancing type specificity and reducing unnecessary conversions.Chores