[ISSUE #4187]♻️Refactor topic configuration handling to use DashMap for improved concurrency #4206

mxsm · 2025-10-25T15:03:51Z

Which Issue(s) This PR Fixes(Closes)

Fixes #4187

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

Refactor
- Updated internal topic configuration storage from mutex-protected HashMap to concurrent DashMap for improved concurrency handling.
- Refactored topic configuration ownership model with ArcMut wrapper for shared mutable references.
- Updated public API signatures for topic configuration methods, including select_topic_config, register_single_topic_all, and related handlers to use new ownership patterns.
- Removed explicit lock acquisition in iterative operations over topic configurations.
- Added serialization support for ArcMut-wrapped configurations.

…or improved concurrency

coderabbitai · 2025-10-25T15:04:03Z

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

📥 Commits

Reviewing files that changed from the base of the PR and between f45dac6 and be21b96.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (24)

rocketmq-broker/src/broker_runtime.rs (5 hunks)
rocketmq-broker/src/hook/batch_check_before_put_message.rs (1 hunks)
rocketmq-broker/src/offset/manager/consumer_order_info_manager.rs (1 hunks)
rocketmq-broker/src/out_api/broker_outer_api.rs (2 hunks)
rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs (8 hunks)
rocketmq-broker/src/processor/notification_processor.rs (1 hunks)
rocketmq-broker/src/processor/pop_message_processor.rs (1 hunks)
rocketmq-broker/src/processor/processor_service/pop_revive_service.rs (1 hunks)
rocketmq-broker/src/processor/send_message_processor.rs (2 hunks)
rocketmq-broker/src/slave/slave_synchronize.rs (2 hunks)
rocketmq-broker/src/topic/manager/topic_config_manager.rs (28 hunks)
rocketmq-broker/src/transaction/queue/transactional_message_bridge.rs (1 hunks)
rocketmq-broker/src/util/hook_utils.rs (4 hunks)
rocketmq-common/src/utils/cleanup_policy_utils.rs (2 hunks)
rocketmq-common/src/utils/queue_type_utils.rs (2 hunks)
rocketmq-remoting/src/protocol/body/create_topic_list_request_body.rs (1 hunks)
rocketmq-remoting/src/protocol/body/topic_info_wrapper.rs (1 hunks)
rocketmq-store/Cargo.toml (1 hunks)
rocketmq-store/src/base/append_message_callback.rs (3 hunks)
rocketmq-store/src/log_file/commit_log.rs (4 hunks)
rocketmq-store/src/message_store/local_file_message_store.rs (7 hunks)
rocketmq-store/src/queue/local_file_consume_queue_store.rs (2 hunks)
rocketmq/Cargo.toml (1 hunks)
rocketmq/src/arc_mut.rs (2 hunks)

 ________________________________
< NVIDIA inside, Rabbit outside. >
 --------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

Tip

You can disable poems in the walkthrough.

Disable the reviews.poem setting in your project's settings in CodeRabbit to disable the poems in the walkthrough.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch refactor-4187

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

rocketmq-rust-bot · 2025-10-25T15:04:05Z

🔊@mxsm 🚀Thanks for your contribution🎉！

💡CodeRabbit(AI) will review your code first🔥！

Note

🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥.

Copilot

Pull Request Overview

This PR refactors topic configuration handling by replacing parking_lot::Mutex<HashMap> with DashMap for improved concurrency. The change eliminates the need for explicit locking when accessing topic configurations, as DashMap provides concurrent access through internal lock striping.

Key changes:

Replaced Arc<parking_lot::Mutex<HashMap<CheetahString, TopicConfig>>> with Arc<DashMap<CheetahString, ArcMut<TopicConfig>>> throughout the codebase
Added Serialize/Deserialize implementations for ArcMut<T> to support the new configuration structure
Created helper functions get_cq_type_arc_mut, is_batch_cq_arc_mut, and get_delete_policy_arc_mut to handle ArcMut<TopicConfig> references

Reviewed Changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
rocketmq/src/arc_mut.rs	Added serde serialization support for ArcMut
rocketmq/Cargo.toml	Added serde dependency
rocketmq-store/src/queue/local_file_consume_queue_store.rs	Updated to use new arc_mut helper functions
rocketmq-store/src/message_store/local_file_message_store.rs	Migrated from Mutex-wrapped HashMap to DashMap
rocketmq-store/src/log_file/commit_log.rs	Updated function signatures for DashMap
rocketmq-store/src/base/append_message_callback.rs	Updated topic_config_table type
rocketmq-store/Cargo.toml	Added dashmap dependency
rocketmq-remoting/src/protocol/body/topic_info_wrapper.rs	Added helper method for taking ownership
rocketmq-remoting/src/protocol/body/create_topic_list_request_body.rs	Changed to use ArcMut
rocketmq-common/src/utils/queue_type_utils.rs	Added arc_mut helper functions
rocketmq-common/src/utils/cleanup_policy_utils.rs	Added arc_mut helper function
rocketmq-broker/src/util/hook_utils.rs	Migrated to DashMap usage
rocketmq-broker/src/transaction/queue/transactional_message_bridge.rs	Updated return type
rocketmq-broker/src/topic/manager/topic_config_manager.rs	Major refactoring to use DashMap instead of Mutex
rocketmq-broker/src/slave/slave_synchronize.rs	Updated synchronization logic
rocketmq-broker/src/processor/*	Multiple processors updated for new types
rocketmq-broker/src/out_api/broker_outer_api.rs	Updated function signatures
rocketmq-broker/src/offset/manager/consumer_order_info_manager.rs	Fixed format specifier
rocketmq-broker/src/hook/batch_check_before_put_message.rs	Migrated to DashMap
rocketmq-broker/src/broker_runtime.rs	Updated for DashMap access patterns

Comments suppressed due to low confidence (1)

rocketmq-broker/src/topic/manager/topic_config_manager.rs:1

Double unwrap() calls can panic without helpful context. The inner try_unwrap().unwrap() will panic if there are other references to the ArcMut, but provides no context about which topic config failed or why. Consider using expect() with descriptive messages or proper error handling.

/*

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-25T15:05:10Z

rocketmq/src/arc_mut.rs

+    where
+        S: Serializer,
+    {
+        let inner_ref = unsafe { &*self.inner.get() };


The serialize implementation uses unsafe code to access the inner value without synchronization. If serialization occurs while another thread is mutating the value through ArcMut, this could lead to undefined behavior. Consider using interior mutability with proper synchronization (e.g., RwLock) or document that serialization must only occur when no concurrent mutations are possible.

Copilot · 2025-10-25T15:05:10Z

rocketmq-broker/src/topic/manager/topic_config_manager.rs

            config.order = is_order;

-            self.put_topic_config(config.clone());
+            let current_ref = self.put_topic_config(config);


The variable current_ref is assigned but the original config variable is shadowed. The function put_topic_config consumes config and returns the previous value (or None), not the newly inserted value. This means current_ref contains the old value that was replaced, not the newly inserted config. The return statement at line 430 should likely return the newly created config instead.

Copilot · 2025-10-25T15:05:10Z

rocketmq-broker/src/topic/manager/topic_config_manager.rs

+            //can optimize todo
+            self.update_topic_config(topic_config.clone());


Each iteration clones the ArcMut<TopicConfig>, which increments the reference count. Since update_topic_config doesn't require ownership of the clone, consider refactoring to avoid unnecessary clones in a loop, or document why the clone is necessary here.

Copilot · 2025-10-25T15:05:11Z

rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs

+            .get_topic_config(topic.as_str())
+            .clone();


The call chain .get_topic_config(topic.as_str()).clone() is redundant. The get_topic_config method already returns Option<ArcMut<TopicConfig>> which clones the ArcMut internally via as_deref().cloned(). The additional .clone() here creates an unnecessary extra reference count increment.

Suggested change

.get_topic_config(topic.as_str())

.clone();

.get_topic_config(topic.as_str());

rocketmq-rust-bot · 2025-10-26T00:52:47Z

🔊@mxsm 🚀Thanks for your contribution🎉！

💡CodeRabbit(AI) will review your code first🔥！

Note

🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥.

codecov · 2025-10-26T00:55:36Z

Codecov Report

❌ Patch coverage is 10.30928% with 174 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.71%. Comparing base (f45dac6) to head (be21b96).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...q-broker/src/topic/manager/topic_config_manager.rs	0.00%	90 Missing ⚠️
rocketmq-common/src/utils/cleanup_policy_utils.rs	0.00%	12 Missing ⚠️
rocketmq/src/arc_mut.rs	0.00%	12 Missing ⚠️
...or/admin_broker_processor/topic_request_handler.rs	0.00%	10 Missing ⚠️
rocketmq-common/src/utils/queue_type_utils.rs	61.11%	7 Missing ⚠️
rocketmq-broker/src/broker_runtime.rs	0.00%	6 Missing ⚠️
rocketmq-broker/src/slave/slave_synchronize.rs	0.00%	6 Missing ⚠️
...tore/src/message_store/local_file_message_store.rs	0.00%	6 Missing ⚠️
rocketmq-broker/src/out_api/broker_outer_api.rs	0.00%	5 Missing ⚠️
rocketmq-store/src/log_file/commit_log.rs	0.00%	5 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4206      +/-   ##
==========================================
- Coverage   26.72%   26.71%   -0.01%     
==========================================
  Files         578      578              
  Lines       82009    82071      +62     
==========================================
+ Hits        21914    21927      +13     
- Misses      60095    60144      +49

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

rocketmq-rust-bot

LGTM

[ISSUE #4187]♻️Refactor topic configuration handling to use DashMap f…

832e99f

…or improved concurrency

Copilot AI review requested due to automatic review settings October 25, 2025 15:03

rocketmq-rust-bot requested review from SpaceXCN, TeslaRustor and rocketmq-rust-bot October 25, 2025 15:04

rocketmq-rust-bot added AI review first Ai review pr first auto merge ready to review waiting-review waiting review this PR labels Oct 25, 2025

rocketmq-rust-robot added the refactor♻️ refactor code label Oct 25, 2025

Copilot AI reviewed Oct 25, 2025

View reviewed changes

♻️Refactor test to use DashMap for topic configuration handling

be21b96

rocketmq-rust-bot approved these changes Oct 26, 2025

View reviewed changes

rocketmq-rust-bot merged commit 9274a64 into main Oct 26, 2025
15 of 18 checks passed

mxsm deleted the refactor-4187 branch October 26, 2025 00:57

rocketmq-rust-bot added approved PR has approved and removed ready to review waiting-review waiting review this PR labels Oct 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[ISSUE #4187]♻️Refactor topic configuration handling to use DashMap for improved concurrency #4206

[ISSUE #4187]♻️Refactor topic configuration handling to use DashMap for improved concurrency #4206

Uh oh!

mxsm commented Oct 25, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 25, 2025 •

edited

Loading

Other AI code review bot(s) detected

Uh oh!

rocketmq-rust-bot commented Oct 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 25, 2025

Uh oh!

Copilot AI Oct 25, 2025

Uh oh!

Copilot AI Oct 25, 2025

Uh oh!

Copilot AI Oct 25, 2025

Uh oh!

rocketmq-rust-bot commented Oct 26, 2025

Uh oh!

codecov bot commented Oct 26, 2025 •

edited

Loading

Uh oh!

rocketmq-rust-bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		//can optimize todo
		self.update_topic_config(topic_config.clone());

	.get_topic_config(topic.as_str())
	.clone();
	.get_topic_config(topic.as_str());

Uh oh!

[ISSUE #4187]♻️Refactor topic configuration handling to use DashMap for improved concurrency #4206

[ISSUE #4187]♻️Refactor topic configuration handling to use DashMap for improved concurrency #4206

Uh oh!

Conversation

mxsm commented Oct 25, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Issue(s) This PR Fixes(Closes)

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Uh oh!

rocketmq-rust-bot commented Oct 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

rocketmq-rust-bot commented Oct 26, 2025

Uh oh!

codecov bot commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rocketmq-rust-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mxsm commented Oct 25, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 25, 2025 •

edited

Loading

codecov bot commented Oct 26, 2025 •

edited

Loading