[ISSUE #2109]💫Refactor name server crate with NameServerRuntimeInner♻️ #2110
Conversation
WalkthroughThe pull request introduces a significant architectural refactoring of the RocketMQ name server runtime. The primary change involves creating a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
rocketmq-rust-bot
requested review from
rocketmq-rust-bot,
SpaceXCN and
TeslaRustor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2110 +/- ##
==========================================
- Coverage 28.26% 28.05% -0.21%
==========================================
Files 493 493
Lines 70675 70722 +47
==========================================
- Hits 19977 19843 -134
- Misses 50698 50879 +181 ☔ View full report in Codecov by Sentry. |
rocketmq-rust-bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
rocketmq-namesrv/src/processor/client_request_processor.rs (1)
117-123: Optional KV config retrieval.
Storingorder_topic_confdirectly is reasonable. For debugging, consider logging if config retrieval returnsNone.rocketmq-namesrv/src/bootstrap.rs (3)
52-60: Commented-out fields and new fields.
Leaving old fields commented might cause confusion. Removing them or using version control history can keep the codebase cleaner.-/*name_server_config: ArcMut<NamesrvConfig>, - ... - remoting_client: ArcMut<RocketmqDefaultClient>,*/ +// Remove commented-out code for clarity unless it's temporarily kept for reference
221-223: Mutable retrieval methods.
Returning.expect("...")inroute_info_manager_mut/kvconfig_manager_mutis direct. Consider returningResultto avoid panics in certain edge cases.Also applies to: 228-230
255-257: Immutable retrieval methods.
Same concern as mutable methods—expect("...")is straightforward but panics if these resources are unexpectedly absent.Also applies to: 262-264
rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs (2)
125-128: Persisting KV configs.
Writing updated data toname_server_runtime_inner.name_server_config().kv_config_path. Consider adding fallback or backup logic if the path is unwritable.
209-213: Test module’s usage of builder.
Commented-out test code referencesBuilder. Re-enable or remove if no longer required.rocketmq-namesrv/src/processor/default_request_processor.rs (4)
303-312: Optional order topic config to broker.
Ifreturn_order_topic_config_to_brokeris enabled, config is included. Verify that large configs won’t cause overhead or timeouts.
453-461: All topic list retrieval.
Checkingenable_all_topic_listis consistent. Potentially large lists might require pagination or streaming in the future.
542-546: Cluster-based topic retrieval.
Disabling the feature with a simple condition is fine. Log if needed.
692-695: Updating NameServer config.
Ensure partial updates remain consistent. Potential rollback mechanism if needed in production.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rocketmq-namesrv/src/bootstrap.rs(7 hunks)rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs(8 hunks)rocketmq-namesrv/src/processor/client_request_processor.rs(4 hunks)rocketmq-namesrv/src/processor/default_request_processor.rs(25 hunks)rocketmq-namesrv/src/route/route_info_manager.rs(8 hunks)rocketmq-remoting/src/runtime/config/client_config.rs(1 hunks)
🔇 Additional comments (50)
rocketmq-namesrv/src/processor/client_request_processor.rs (7)
36-36: Import addition looks appropriate.
No functional or security concerns.
42-42: Struct field consolidation enhances encapsulation.
Replacing multiple fields (route_info_manager, etc.) withname_server_runtime_innersimplifies dependency management.
48-52: Constructor refactor is clear and consistent.
Accepting a singleNameServerRuntimeInnerparameter ensures a cleaner interface and reduces code duplication.
70-75: Time calculation logic.
Casting to milliseconds and comparing the difference is valid. However, consider verifying largewait_seconds_for_servicevalues are properly handled to avoid potential overflow.
76-81: Conditional server readiness check.
Logic is consistent but ensure concurrency safety if other threads also check or setneed_check_namesrv_ready. Atomic usage appears correct.
94-95: Safe topic route retrieval.
No issues found usingpickup_topic_route_data. Check upstream handling of theOptionreturned.
111-115: Conditional ordering configuration.
Usingorder_message_enablefor gating special processing seems correct.rocketmq-namesrv/src/bootstrap.rs (9)
79-86: Server startup logic.
CreatingRocketMQServerwithserver_config.listen_portis straightforward. Confirm that the correct port is used for all environments.
88-93: Weak reference usage for remoting client.
UsingArcMut::downgradeis a typical pattern to prevent cyclic references. Ensure that any calls onremoting_clientare preceded by validity checks.
101-117: Initialize processors & schedule tasks.
Invokingroute_info_manager().start(...)and schedulingscan_not_active_brokeris cohesive. Watch out for potential concurrency bottlenecks in broadcast usage.
162-166: Builder pattern usage.
Fallback to default configs is logical forNamesrvConfigandTokioClientConfig. This approach is consistent with robust code design.
169-177: ConstructingNameServerRuntimeInner.
Centralizing configurations (name_server_config,tokio_client_config, etc.) fosters better maintainability.
179-182: Attaching managers to the inner field.
Assigningroute_info_managerandkvconfig_managerasSome(...)ensures they remain optional. Confirm that no usage occurs before initialization.
188-188: Storinginnerreference.
Clean integration of the newly-createdArcMut<NameServerRuntimeInner>intoNameServerRuntime.
198-200: Optional typed fields inNameServerRuntimeInner.
The optional approach is flexible, but ensure you call.expect("...")carefully, or handleNonegracefully in production.
289-289: Setters for runtime inner fields.
Implementation looks consistent. Optionally, add checks/logs to confirm valid arguments.Also applies to: 294-294, 298-298
rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs (7)
31-31: ImportingNameServerRuntimeInner.
No immediate concerns. Consolidation aligns with the PR’s refactor objective.
44-44: Single consolidated field.
Replacingnamesrv_configwithname_server_runtime_innersimplifies internal usage.
57-59: Redesigned constructor.
Now only requiresArcMut<NameServerRuntimeInner>. This is simpler for testing but ensure that any direct config usage is updated.Also applies to: 62-62
83-83: Get Namesrv config fromNameServerRuntimeInner.
Direct accessor usage is consistent with the new design.
90-95: Loading KV configs from file.
Readingname_server_config().kv_config_pathis correct. Confirm file handling with robust error checking (though partial checks are present).
112-114: Updating config in place.
name_server_config_mut().update(...)is a direct call. Any errors are returned. This is cohesive with the new design.
217-217: Combined usage in tests.
builder.build()returningname_server_runtime_inner.kvconfig_manager()is more integrated. Possibly confirm no race conditions if tests are run in parallel.rocketmq-namesrv/src/processor/default_request_processor.rs (19)
60-60: ArcMut import usage.
Consistent with the rest of the refactor. No issues noted.
63-63: Singlename_server_runtime_innerfield.
Combining the previous managers into this single reference is neat and reduces boilerplate.Also applies to: 69-69
138-144: KV config insertion uses new runtime inner.
Ensure correct concurrency practice when updating shared data input_kv_config.
159-160: Synchronous KV retrieval.
No issues in reading fromkvconfig_manager. The code returns an error response if missing.
188-189: KV config deletion.
Again, concurrency correctness is crucial. The usage of locks insideKVConfigManagerlooks appropriate.
208-216: Topic config change detection.
Refactored approach vianame_server_runtime_inneris logical. Double-check data version logic to prevent stale updates.Also applies to: 217-222, 226-226
238-240: Consolidated constructor.
DefaultRequestProcessor::new(...)now only requiresArcMut<NameServerRuntimeInner>. Matches the rest of the refactor.
279-296: Broker registration logic.
Storing broker info viaroute_info_manager().register_brokeris consistent with the new structure. Ensure the snapshot or data version mismatch is handled gracefully.
338-339: Broker unregistration.
Refactored to callroute_info_manager_mut().un_register_broker. No issues.
358-363: Broker heartbeat update.
route_info_manager_mut().update_broker_info_update_timestamp(...)is straightforward.
381-382: Get broker member group.
Relies onroute_info_manager_mut(). Confirm no conflicts if called in parallel with other route updates.
398-399: Getting broker cluster info.
Reads fromroute_info_manager().get_all_cluster_info(); no concerns found.
422-423: Modifying broker write permissions.
wipe_write_perm_of_broker_by_lockandadd_write_perm_of_broker_by_lockhandle locking internally. Good concurrency approach.Also applies to: 442-443
487-488: Deleting topic in NameServer.
Synchronized withroute_info_manager_mut().delete_topic(...). No further concerns.
508-509: Register topic in NameServer.
Creates or updates existing queue definitions inroute_info_manager. No issues observed.
526-527: Namespace-based KV listing.
Method is consistent with the updated manager pattern.
575-578: System topic and unit topic retrieval logic.
All are straightforward read operations. No concurrency or correctness issues apparent.Also applies to: 586-594, 612-620, 638-645
682-683: Blacklist config check.
Ensuring certain properties can’t be updated is a valid security measure.
711-711: Get entire config.
Successfully retrieves config fromname_server_runtime_inner.name_server_config().rocketmq-namesrv/src/route/route_info_manager.rs (7)
52-52: ImportingNameServerRuntimeInner.
Matches the PR’s overarching refactor.
83-83: Unified field inRouteInfoManager.
name_server_runtime_innerhelps centralize config & remoting client usage.
89-98: Constructor uses consolidated field.
This is consistent with other managers. Ensures shared state is managed via a single entry point.
235-238: Topic deletion upon broker registration.
Conditionaldelete_topic_with_broker_registrationusage might surprise some brokers. Confirm these topics are truly obsolete or consider a safer approach.
346-351: Notify min broker ID changes.
Checks thenotify_min_broker_id_changedconfig. The approach is correct but confirm potential concurrency issues if multiple concurrent calls.
1033-1038: Notification after broker unregistration.
Again checksnotify_min_broker_id_changed. The logic is consistent with the new design.
1163-1170: Asynchronous socket disconnect listener.
Usestokio::spawnreferencing a clonedArcMut<NameServerRuntimeInner>. Validate that potential simultaneous writes to route info are properly synchronized.rocketmq-remoting/src/runtime/config/client_config.rs (1)
26-26: Enabling clone is beneficial for flexible runtime usage and configuration.Adding
#[derive(Clone)]here is straightforward and allowsTokioClientConfigto be duplicated as needed. Given that all fields are either primitives orString, this clone implementation should pose no immediate performance or concurrency concerns. Good to see that no references or synchronization primitives are involved.
Which Issue(s) This PR Fixes(Closes)
Fixes #2109
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Refactor
NameServerRuntimeto improve configuration and client managementNameServerRuntimeInnerstructNew Features
TokioClientConfigChores