feat/91: Add get namesrv config request #1485
Conversation
WalkthroughThe pull request introduces enhancements to the 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
|
|
🔊@PanGan21 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
rocketmq-rust-robot
added
enhancement⚡️
| pub fn get_all_configs_format_string(&self) -> Result<String, String> { | ||
| let mut json_map = HashMap::new(); | ||
|
|
||
| json_map.insert( |
There was a problem hiding this comment.
This can be improved if we replace #[serde(alias = "property")] with #[serde(rename = "property")].
Then we can use serde_json here, but I am not sure how many things this change would break.
|
🔊@PanGan21 🚀Thanks for your contribution 🎉. 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: 1
🧹 Outside diff range and nitpick comments (1)
rocketmq-common/src/common/namesrv/namesrv_config.rs (1)
152-248: Consider simplifying the implementation using direct serialization.The current implementation manually constructs a HashMap and converts each field. Since the struct already derives
Serialize, you could simplify this by directly serializing the struct instance.Consider this simpler implementation:
- pub fn get_all_configs_format_string(&self) -> Result<String, String> { - let mut json_map = HashMap::new(); - - json_map.insert( - "rocketmqHome".to_string(), - Value::String(self.rocketmq_home.clone()), - ); - // ... (removed other insertions for brevity) - - // Convert the HashMap to a JSON value - match serde_json::to_string_pretty(&json_map) { - Ok(json) => Ok(json), - Err(err) => { - return Err(format!("Failed to serialize NamesrvConfig: {}", err)); - } - } - } + pub fn get_all_configs_format_string(&self) -> Result<String, String> { + serde_json::to_string_pretty(self) + .map_err(|err| format!("Failed to serialize NamesrvConfig: {}", err)) + }This approach:
- Leverages the derived
Serializeimplementation- Reduces code duplication
- Maintains the same functionality
- Is less prone to errors when struct fields change
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-common/src/common/namesrv/namesrv_config.rs(3 hunks)rocketmq-namesrv/src/processor/default_request_processor.rs(2 hunks)
🔇 Additional comments (3)
rocketmq-common/src/common/namesrv/namesrv_config.rs (2)
23-23: LGTM: Serialize trait addition is correct.
The addition of the Serialize trait is appropriate for enabling JSON serialization of the NamesrvConfig struct.
Also applies to: 29-29
538-604: LGTM: Comprehensive test coverage.
The test case thoroughly verifies:
- JSON serialization success
- Non-empty output
- Correct serialization of all config fields
rocketmq-namesrv/src/processor/default_request_processor.rs (1)
109-109: LGTM: Request code handling is correctly implemented.
The addition of RequestCode::GetNamesrvConfig handler is consistent with the existing pattern and correctly maps to the new get_config method.
| fn get_config(&mut self, _request: RemotingCommand) -> RemotingCommand { | ||
| let config = self.kvconfig_manager.get_namesrv_config(); | ||
| match config.get_all_configs_format_string() { | ||
| Ok(content) => { | ||
| let response = RemotingCommand::create_response_command_with_code_remark( | ||
| RemotingSysResponseCode::Success, | ||
| CheetahString::empty(), | ||
| ); | ||
| response.set_body(content.into_bytes()) | ||
| } | ||
| Err(e) => RemotingCommand::create_response_command_with_code_remark( | ||
| ResponseCode::SystemError, | ||
| format!("UnsupportedEncodingException " + e), | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix string concatenation in error message.
The error message formatting uses the + operator which is less idiomatic in Rust.
Apply this change:
Err(e) => RemotingCommand::create_response_command_with_code_remark(
ResponseCode::SystemError,
- format!("UnsupportedEncodingException " + e),
+ format!("UnsupportedEncodingException {}", e),
),This change:
- Uses the more idiomatic string formatting in Rust
- Maintains the same error message content
- Is consistent with other error handling in the codebase
📝 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.
| fn get_config(&mut self, _request: RemotingCommand) -> RemotingCommand { | |
| let config = self.kvconfig_manager.get_namesrv_config(); | |
| match config.get_all_configs_format_string() { | |
| Ok(content) => { | |
| let response = RemotingCommand::create_response_command_with_code_remark( | |
| RemotingSysResponseCode::Success, | |
| CheetahString::empty(), | |
| ); | |
| response.set_body(content.into_bytes()) | |
| } | |
| Err(e) => RemotingCommand::create_response_command_with_code_remark( | |
| ResponseCode::SystemError, | |
| format!("UnsupportedEncodingException " + e), | |
| ), | |
| } | |
| } | |
| fn get_config(&mut self, _request: RemotingCommand) -> RemotingCommand { | |
| let config = self.kvconfig_manager.get_namesrv_config(); | |
| match config.get_all_configs_format_string() { | |
| Ok(content) => { | |
| let response = RemotingCommand::create_response_command_with_code_remark( | |
| RemotingSysResponseCode::Success, | |
| CheetahString::empty(), | |
| ); | |
| response.set_body(content.into_bytes()) | |
| } | |
| Err(e) => RemotingCommand::create_response_command_with_code_remark( | |
| ResponseCode::SystemError, | |
| format!("UnsupportedEncodingException {}", e), | |
| ), | |
| } | |
| } |
|
🔊@PanGan21 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
|
🔊@PanGan21 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
rocketmq-common/src/common/namesrv/namesrv_config.rs (1)
535-601: Enhance test coverageThe test thoroughly verifies successful serialization but could be improved with additional test cases.
Consider adding:
- Error case testing:
#[test] fn test_get_all_configs_format_string_error() { // Mock a scenario that would cause serialization to fail // This might require creating a test-specific struct with custom serialization // that always fails }
- Use test constants for better maintainability:
#[cfg(test)] mod tests { const TEST_CONFIG: &str = r#"{ "rocketmqHome": "/path/to/home", "kvConfigPath": "/path/to/kv", // ... other fields ... }"#; #[test] fn test_get_all_configs_format_string_matches_expected() { let config = NamesrvConfig::new(); let json_output = config.get_all_configs_format_string().unwrap(); // Compare with expected format assert_eq!( serde_json::from_str::<serde_json::Value>(&json_output).unwrap(), serde_json::from_str::<serde_json::Value>(TEST_CONFIG).unwrap() ); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-common/src/common/namesrv/namesrv_config.rs(3 hunks)rocketmq-namesrv/src/processor/default_request_processor.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rocketmq-namesrv/src/processor/default_request_processor.rs
🔇 Additional comments (2)
rocketmq-common/src/common/namesrv/namesrv_config.rs (2)
23-23: LGTM!
The serde_json::Value import is appropriately placed and necessary for the new configuration serialization functionality.
Line range hint 29-29: Consider using serde(rename) instead of serde(alias)
As mentioned in a previous review, consider replacing #[serde(alias = "property")] with #[serde(rename = "property")] throughout the struct. This would allow using serde_json directly and potentially simplify the implementation.
| pub fn get_all_configs_format_string(&self) -> Result<String, String> { | ||
| let mut json_map = HashMap::new(); | ||
|
|
||
| json_map.insert( | ||
| "rocketmqHome".to_string(), | ||
| Value::String(self.rocketmq_home.clone()), | ||
| ); | ||
| json_map.insert( | ||
| "kvConfigPath".to_string(), | ||
| Value::String(self.kv_config_path.clone()), | ||
| ); | ||
| json_map.insert( | ||
| "configStorePath".to_string(), | ||
| Value::String(self.config_store_path.clone()), | ||
| ); | ||
| json_map.insert( | ||
| "productEnvName".to_string(), | ||
| Value::String(self.product_env_name.clone()), | ||
| ); | ||
| json_map.insert("clusterTest".to_string(), Value::Bool(self.cluster_test)); | ||
| json_map.insert( | ||
| "orderMessageEnable".to_string(), | ||
| Value::Bool(self.order_message_enable), | ||
| ); | ||
| json_map.insert( | ||
| "returnOrderTopicConfigToBroker".to_string(), | ||
| Value::Bool(self.return_order_topic_config_to_broker), | ||
| ); | ||
| json_map.insert( | ||
| "clientRequestThreadPoolNums".to_string(), | ||
| Value::Number(self.client_request_thread_pool_nums.into()), | ||
| ); | ||
| json_map.insert( | ||
| "defaultThreadPoolNums".to_string(), | ||
| Value::Number(self.default_thread_pool_nums.into()), | ||
| ); | ||
| json_map.insert( | ||
| "clientRequestThreadPoolQueueCapacity".to_string(), | ||
| Value::Number(self.client_request_thread_pool_queue_capacity.into()), | ||
| ); | ||
| json_map.insert( | ||
| "defaultThreadPoolQueueCapacity".to_string(), | ||
| Value::Number(self.default_thread_pool_queue_capacity.into()), | ||
| ); | ||
| json_map.insert( | ||
| "scanNotActiveBrokerInterval".to_string(), | ||
| Value::Number(self.scan_not_active_broker_interval.into()), | ||
| ); | ||
| json_map.insert( | ||
| "unRegisterBrokerQueueCapacity".to_string(), | ||
| Value::Number(self.unregister_broker_queue_capacity.into()), | ||
| ); | ||
| json_map.insert( | ||
| "supportActingMaster".to_string(), | ||
| Value::Bool(self.support_acting_master), | ||
| ); | ||
| json_map.insert( | ||
| "enableAllTopicList".to_string(), | ||
| Value::Bool(self.enable_all_topic_list), | ||
| ); | ||
| json_map.insert( | ||
| "enableTopicList".to_string(), | ||
| Value::Bool(self.enable_topic_list), | ||
| ); | ||
| json_map.insert( | ||
| "notifyMinBrokerIdChanged".to_string(), | ||
| Value::Bool(self.notify_min_broker_id_changed), | ||
| ); | ||
| json_map.insert( | ||
| "enableControllerInNamesrv".to_string(), | ||
| Value::Bool(self.enable_controller_in_namesrv), | ||
| ); | ||
| json_map.insert( | ||
| "needWaitForService".to_string(), | ||
| Value::Bool(self.need_wait_for_service), | ||
| ); | ||
| json_map.insert( | ||
| "waitSecondsForService".to_string(), | ||
| Value::Number(self.wait_seconds_for_service.into()), | ||
| ); | ||
| json_map.insert( | ||
| "deleteTopicWithBrokerRegistration".to_string(), | ||
| Value::Bool(self.delete_topic_with_broker_registration), | ||
| ); | ||
| json_map.insert( | ||
| "configBlackList".to_string(), | ||
| Value::String(self.config_black_list.clone()), | ||
| ); | ||
|
|
||
| // Convert the HashMap to a JSON value | ||
| match serde_json::to_string_pretty(&json_map) { | ||
| Ok(json) => Ok(json), | ||
| Err(err) => Err(format!("Failed to serialize NamesrvConfig: {}", err)), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify implementation by deriving Serialize trait
The current implementation manually constructs a HashMap for serialization, which is verbose and potentially error-prone. Since the struct already uses serde attributes, we can simplify this significantly by deriving the Serialize trait.
- Add
Serializeto the derive attributes:
-#[derive(Debug, Clone, Deserialize)]
+#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct NamesrvConfig {- Simplify the implementation:
pub fn get_all_configs_format_string(&self) -> Result<String, String> {
- let mut json_map = HashMap::new();
- json_map.insert("rocketmqHome".to_string(), Value::String(self.rocketmq_home.clone()));
- // ... remove all other manual insertions ...
-
- match serde_json::to_string_pretty(&json_map) {
+ match serde_json::to_string_pretty(&self) {
Ok(json) => Ok(json),
Err(err) => Err(format!("Failed to serialize NamesrvConfig: {}", err)),
}
}This approach:
- Reduces code duplication
- Eliminates potential manual mapping errors
- Automatically handles new fields added to the struct
- Maintains consistent serialization with the existing serde attributes
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This will not work unless we replace alias derive with rename. But in case this is done there might be breaking changes, so I am not sure
There was a problem hiding this comment.
@PanGan21, understood. Thank you for clarifying. Indeed, changing alias to rename in serde attributes might introduce breaking changes. Keeping the current implementation makes sense to preserve backwards compatibility.
✏️ Learnings added
Learnt from: PanGan21
PR: mxsm/rocketmq-rust#1485
File: rocketmq-common/src/common/namesrv/namesrv_config.rs:151-245
Timestamp: 2024-12-01T10:26:22.095Z
Learning: In `NamesrvConfig`, replacing `serde(alias = "...")` with `serde(rename = "...")` could cause breaking changes, so manual serialization is preferred to avoid disrupting existing functionality.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1485 +/- ##
==========================================
+ Coverage 22.34% 22.53% +0.18%
==========================================
Files 450 450
Lines 58066 58418 +352
==========================================
+ Hits 12974 13162 +188
- Misses 45092 45256 +164 ☔ View full report in Codecov by Sentry. |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #91
Brief Description
Adds get namsrv config request
How Did You Test This Change?
Unit tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests