[ISSUE #90]Support update namesrv config #1316
Conversation
WalkthroughThe changes introduce several new methods and functionalities across multiple files in the RocketMQ codebase. A function for parsing key-value pairs from strings into a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@PanGan21 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 38 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
|
|
🔊@PanGan21 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review 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 (13)
rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs (1)
99-106: Add comprehensive documentation for the new method.The
update_namesrv_configmethod lacks documentation explaining its purpose, parameters, and possible error cases. Consider adding rustdoc comments following the codebase style.Apply this diff to add documentation:
+ /// Updates the Namesrv configuration with the provided key-value pairs. + /// + /// # Arguments + /// + /// * `updates` - A HashMap containing configuration keys and their new values + /// + /// # Returns + /// + /// * `Ok(())` if the update was successful + /// * `Err(String)` containing the error message if the update failed pub fn update_namesrv_config( &mut self, updates: HashMap<CheetahString, CheetahString>, ) -> Result<(), String> { self.namesrv_config.update(updates) }rocketmq-common/src/common/mix_all.rs (2)
163-184: Consider adding input validation and safety checks.While the basic implementation is correct, consider adding these safety improvements:
- Validate maximum input size to prevent memory exhaustion
- Add key/value content validation
- Handle duplicate keys (either error out or document the override behavior)
pub fn string_to_properties(input: &str) -> Option<HashMap<CheetahString, CheetahString>> { + // Prevent DoS through memory exhaustion + const MAX_INPUT_SIZE: usize = 1024 * 1024; // 1MB limit + if input.len() > MAX_INPUT_SIZE { + return None; + } + let mut properties = HashMap::new(); for line in input.lines() { let line = line.trim(); + // Prevent extremely long lines + if line.len() > 1024 { + return None; + } if line.is_empty() || line.starts_with('#') { // Skip empty lines or comments continue; } if let Some((key, value)) = line.split_once('=') { + let key = key.trim(); + let value = value.trim(); + + // Validate key/value content + if key.is_empty() || !key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '.') { + return None; + } + // Convert key and value to CheetahString - let key = CheetahString::from(key.trim()); - let value = CheetahString::from(value.trim()); + let key = CheetahString::from(key); + let value = CheetahString::from(value); properties.insert(key, value); } else { return None; // Return None if the line isn't in `key=value` format } } Some(properties) }
269-296: Add more test cases for edge cases and error conditions.The current test coverage should be expanded to include:
- Empty input
- Input with only comments/empty lines
- Duplicate keys behavior
- Maximum input size limits
- Special characters and Unicode handling
#[test] fn test_string_to_properties_empty_input() { let input = ""; let result = string_to_properties(input).expect("Empty input should be valid"); assert!(result.is_empty()); } #[test] fn test_string_to_properties_only_comments() { let input = "# comment1\n # comment2 \n\n"; let result = string_to_properties(input).expect("Comments-only input should be valid"); assert!(result.is_empty()); } #[test] fn test_string_to_properties_duplicate_keys() { let input = "key1=value1\nkey1=value2"; let result = string_to_properties(input).expect("Duplicate keys should use last value"); assert_eq!(result.len(), 1); assert_eq!(result.get(&CheetahString::from("key1")), Some(&CheetahString::from("value2"))); } #[test] fn test_string_to_properties_special_chars() { let input = "key.1=value@1\nkey_2=value#2"; let result = string_to_properties(input); // Update assertion based on your validation requirements assert!(result.is_some()); }rocketmq-remoting/src/code/response_code.rs (1)
Line range hint
59-59: Consider adding documentation for the NoPermission variant.While the code is correct, adding documentation comments explaining when this error code is returned would improve maintainability.
Example documentation:
+ /// Returned when the client lacks sufficient permissions to perform the requested operation, + /// particularly during configuration updates containing blacklisted keys. NoPermission = 16,rocketmq-common/src/common/namesrv/namesrv_config.rs (3)
150-156: Inconsistent Naming: Useget_config_black_listfor ConsistencyThe method name
get_config_blacklistis inconsistent with the fieldconfig_black_list. For better readability and consistency, consider renaming the method toget_config_black_list.
158-260: RefactorupdateMethod to Reduce Code DuplicationThe
updatemethod contains repetitive code for parsing and assigning values. Refactoring can enhance maintainability and readability.Consider the following refactor:
pub fn update( &mut self, properties: HashMap<CheetahString, CheetahString>, ) -> Result<(), String> { for (key, value) in properties { match key.as_str() { "rocketmqHome" => self.rocketmq_home = value.to_string(), "kvConfigPath" => self.kv_config_path = value.to_string(), "configStorePath" => self.config_store_path = value.to_string(), "productEnvName" => self.product_env_name = value.to_string(), "configBlackList" => self.config_black_list = value.to_string(), _ => { if let Some(bool_field) = match key.as_str() { "clusterTest" => Some(&mut self.cluster_test), "orderMessageEnable" => Some(&mut self.order_message_enable), "returnOrderTopicConfigToBroker" => Some(&mut self.return_order_topic_config_to_broker), "supportActingMaster" => Some(&mut self.support_acting_master), "enableAllTopicList" => Some(&mut self.enable_all_topic_list), "enableTopicList" => Some(&mut self.enable_topic_list), "notifyMinBrokerIdChanged" => Some(&mut self.notify_min_broker_id_changed), "enableControllerInNamesrv" => Some(&mut self.enable_controller_in_namesrv), "needWaitForService" => Some(&mut self.need_wait_for_service), "deleteTopicWithBrokerRegistration" => Some(&mut self.delete_topic_with_broker_registration), _ => None, } { *bool_field = value .parse() .map_err(|_| format!("Invalid boolean value for key '{}'", key))?; } else if let Some(int_field) = match key.as_str() { "clientRequestThreadPoolNums" => Some(&mut self.client_request_thread_pool_nums), "defaultThreadPoolNums" => Some(&mut self.default_thread_pool_nums), "clientRequestThreadPoolQueueCapacity" => Some(&mut self.client_request_thread_pool_queue_capacity), "defaultThreadPoolQueueCapacity" => Some(&mut self.default_thread_pool_queue_capacity), "scanNotActiveBrokerInterval" => Some(&mut self.scan_not_active_broker_interval), "unRegisterBrokerQueueCapacity" => Some(&mut self.unregister_broker_queue_capacity), "waitSecondsForService" => Some(&mut self.wait_seconds_for_service), _ => None, } { *int_field = value .parse() .map_err(|_| format!("Invalid integer value for key '{}'", key))?; } else { return Err(format!("Unknown configuration key: '{}'", key)); } } } } Ok(()) }This refactor groups similar operations, reducing repetition and making the code more maintainable.
321-436: Expand Unit Tests to Cover Error ScenariosThe
test_namesrv_config_updatefunction only tests successful updates. To ensure robustness, consider adding tests for error cases, such as invalid values and unknown keys.Example test for an invalid boolean value:
#[test] fn test_namesrv_config_update_invalid_boolean() { let mut config = NamesrvConfig::new(); let mut properties = HashMap::new(); properties.insert( CheetahString::from("clusterTest"), CheetahString::from("not_a_boolean"), ); let result = config.update(properties); assert!(result.is_err()); assert_eq!( result.err().unwrap(), "Invalid boolean value for key 'clusterTest'" ); }Example test for an unknown key:
#[test] fn test_namesrv_config_update_unknown_key() { let mut config = NamesrvConfig::new(); let mut properties = HashMap::new(); properties.insert( CheetahString::from("unknownKey"), CheetahString::from("some_value"), ); let result = config.update(properties); assert!(result.is_err()); assert_eq!( result.err().unwrap(), "Unknown configuration key: 'unknownKey'" ); }rocketmq-namesrv/src/processor/default_request_processor.rs (6)
444-454: Consider using more specific error codes for encoding errorsCurrently, when
str::from_utf8fails to parse the request body, the method returnsRemotingSysResponseCode::SystemError. Using a more specific error code or creating a custom error code for encoding errors can aid in debugging and provide clearer feedback to the client.Apply this diff to return a more appropriate error code:
fn update_config(&mut self, request: RemotingCommand) -> RemotingCommand { if let Some(body) = request.body() { let body_str = match str::from_utf8(&body) { Ok(s) => s, Err(e) => { return RemotingCommand::create_response_command_with_code( - RemotingSysResponseCode::SystemError, + RemotingSysResponseCode::EncodingError, ) .set_remark(format!("UTF-8 decoding error: {:?}", e)); } };
456-464: Improve error handling when parsing propertiesWhen
string_to_propertiesfails, the method returns a genericSystemError. Consider providing a more descriptive error code or message to help identify parsing issues more effectively.Apply this diff for a clearer error message:
let properties = match string_to_properties(body_str) { Some(props) => props, None => { return RemotingCommand::create_response_command_with_code( - RemotingSysResponseCode::SystemError, + RemotingSysResponseCode::PropertyParsingError, ) - .set_remark("string_to_properties error".to_string()); + .set_remark("Failed to parse properties from request body.".to_string()); } };
478-484: Handle errors fromupdate_namesrv_configmore gracefullyWhen
update_namesrv_configreturns an error, the current implementation returns aSystemErrorwith the error message. Consider handling different kinds of errors separately to provide more specific feedback. For example, distinguish between I/O errors, permission errors, or invalid configurations.
543-553: Simplifyvalidate_blacklist_config_existusing iterator methodsYou can improve readability by using the
anymethod to check if any blacklisted key exists in the properties.Apply this diff to refactor the function:
fn validate_blacklist_config_exist( properties: &HashMap<CheetahString, CheetahString>, config_blacklist: &[CheetahString], ) -> bool { - for black_config in config_blacklist { - if properties.contains_key(black_config.as_str()) { - return true; - } - } - false + config_blacklist + .iter() + .any(|black_config| properties.contains_key(black_config.as_str())) }
443-491: Add unit tests forupdate_configmethodCurrently, there are no unit tests covering the
update_configmethod. Adding tests will ensure that the method handles various scenarios correctly, such as valid updates, encoding errors, property parsing failures, and blacklist violations.
443-491: Consider logging configuration updates for audit purposesFor better traceability and debugging, especially in production environments, consider adding logging statements when configurations are successfully updated or when an update fails due to blacklist violations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
rocketmq-common/src/common/mix_all.rs(3 hunks)rocketmq-common/src/common/namesrv/namesrv_config.rs(3 hunks)rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs(1 hunks)rocketmq-namesrv/src/processor/default_request_processor.rs(4 hunks)rocketmq-remoting/src/code/response_code.rs(2 hunks)
🔇 Additional comments (6)
rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs (1)
99-106: Verify the update implementation's thread safety.
The method uses &mut self which ensures exclusive access during updates. However, let's verify there are no potential deadlocks when used with other methods.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information about the thread safety aspects of the namesrv_config usage:
Thread safety is properly implemented for namesrv_config updates
The implementation is thread-safe because:
- The
namesrv_configfield is wrapped inArcMut<NamesrvConfig>, providing atomic reference counting and interior mutability - The
update_namesrv_configmethod requires&mut self, ensuring exclusive access during updates - All other usages of
namesrv_configacross the codebase are read-only operations through immutable references - There are no concurrent modifications observed in the codebase that could lead to potential deadlocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other methods that might interact with namesrv_config
# to ensure proper synchronization
echo "Searching for namesrv_config usage patterns..."
rg -A 5 "namesrv_config" --type rust
echo "Searching for potential deadlock patterns..."
ast-grep --pattern 'fn $method(&mut self) {
$$$
self.namesrv_config.$$$
$$$
}'
Length of output: 24644
rocketmq-common/src/common/mix_all.rs (1)
163-184: Verify the function's usage in configuration updates.
Let's verify how this function is used in the configuration update flow to ensure proper error handling of invalid inputs.
✅ Verification successful
This will help us understand the actual usage context and error handling patterns around the function.
Based on the results, I can now generate the final response:
Error handling for invalid inputs is properly implemented in the configuration flow
The string_to_properties function is used in the namesrv processor with appropriate error handling:
- Returns
Nonefor invalid inputs which is properly handled by returning a system error response - The error message "string_to_properties error" is included in the response
- The function is used in a controlled context where configuration validation (blacklist) is performed afterward
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usage of string_to_properties in configuration update flow
# Search for direct usage of string_to_properties
echo "Direct usage of string_to_properties:"
rg -l "string_to_properties"
# Search for configuration update related code
echo -e "\nConfiguration update related code:"
rg -l "update.*config|config.*update"
# Search for error handling patterns around the usage
echo -e "\nError handling patterns:"
rg -A 5 -B 5 "string_to_properties.*None"
Length of output: 998
Script:
#!/bin/bash
# Let's check how string_to_properties is used in the namesrv processor
echo "Usage in namesrv processor:"
rg -A 10 -B 10 "string_to_properties" rocketmq-namesrv/src/processor/default_request_processor.rs
# Let's also check the configuration update handlers in broker
echo -e "\nConfiguration update handlers in broker:"
rg -A 10 "fn.*update.*config" rocketmq-broker/src/processor/admin_broker_processor/broker_config_request_handler.rs
Length of output: 2799
rocketmq-remoting/src/code/response_code.rs (2)
Line range hint 192-192: LGTM! Test coverage is comprehensive.
The test coverage for the new NoPermission variant is thorough, including both the normal case and edge cases in the conversion tests.
Also applies to: 193-193, 267-267
24-24: LGTM! Verify alignment with RocketMQ protocol.
The addition of NoPermission = 16 to RemotingSysResponseCode and its corresponding From<i32> implementation is correct and follows the established pattern.
Let's verify this matches the Java implementation:
Also applies to: 41-41
rocketmq-common/src/common/namesrv/namesrv_config.rs (1)
199-201:
Ensure Proper Parsing of Unsigned Integer Values
The field scan_not_active_broker_interval is a u64, but the current parsing method does not prevent negative values from being parsed as valid. This could lead to unexpected behavior.
Modify the parsing to explicitly handle unsigned integers:
"scanNotActiveBrokerInterval" => {
self.scan_not_active_broker_interval = value
- .parse()
+ .parse::<u64>()
.map_err(|_| format!("Invalid unsigned integer value for key '{}'", key))?
}Likely invalid or redundant comment.
rocketmq-namesrv/src/processor/default_request_processor.rs (1)
443-491: Ensure consistent handling of empty request bodies
The update_config method returns Success even when the request body is None, resulting in no configuration update. Verify if this is the intended behavior or if the method should return an error when the request body is absent.
| /// Updates the Namesrv configuration. | ||
| pub fn update_namesrv_config( | ||
| &mut self, | ||
| updates: HashMap<CheetahString, CheetahString>, | ||
| ) -> Result<(), String> { | ||
| self.namesrv_config.update(updates) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for the new configuration update functionality.
The new update_namesrv_config method lacks test coverage. Consider adding tests to verify successful updates and error handling.
Add these test cases to the tests module:
#[test]
fn update_namesrv_config_successful() {
let mut manager = create_kv_config_manager();
let mut updates = HashMap::new();
updates.insert("key".into(), "value".into());
let result = manager.update_namesrv_config(updates);
assert!(result.is_ok());
}
#[test]
fn update_namesrv_config_handles_errors() {
let mut manager = create_kv_config_manager();
let mut updates = HashMap::new();
// Add invalid configuration that would cause an error
updates.insert("invalid_key".into(), "invalid_value".into());
let result = manager.update_namesrv_config(updates);
assert!(result.is_err());
}
| .collect() | ||
| } | ||
|
|
||
| pub fn update( |
There was a problem hiding this comment.
Probably this implementation could be improved by using deserialize_with derive?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1316 +/- ##
==========================================
+ Coverage 19.50% 19.82% +0.32%
==========================================
Files 434 434
Lines 54232 54530 +298
==========================================
+ Hits 10576 10810 +234
- Misses 43656 43720 +64 ☔ View full report in Codecov by Sentry. |
|
🔊@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-namesrv/src/processor/default_request_processor.rs (1)
541-551: Make the implementation more idiomaticThe function could be more idiomatic by:
- Renaming to
has_blacklisted_config- Using Iterator::any for a more concise implementation
Consider this refactoring:
-fn validate_blacklist_config_exist( +fn has_blacklisted_config( properties: &HashMap<CheetahString, CheetahString>, config_blacklist: &[CheetahString], ) -> bool { - for black_config in config_blacklist { - if properties.contains_key(black_config.as_str()) { - return true; - } - } - false + config_blacklist.iter().any(|config| properties.contains_key(config.as_str())) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
rocketmq-namesrv/src/processor/default_request_processor.rs(4 hunks)
🔇 Additional comments (2)
rocketmq-namesrv/src/processor/default_request_processor.rs (2)
18-19: LGTM: Clean integration of new functionality
The new imports and match arm are correctly integrated into the existing code structure.
Also applies to: 108-108
444-489: Verify configuration update scenarios
Please ensure that all configuration update scenarios are covered:
- Valid configuration updates
- Updates with blacklisted keys
- Invalid UTF-8 handling
- Empty or malformed properties
✅ Verification successful
Configuration update implementation is well-covered
The implementation adequately handles all key configuration update scenarios:
- Invalid UTF-8 encoding in request body
- Malformed properties parsing
- Blacklist configuration validation
- Update operation failures
- Success path with empty remark
The error handling is comprehensive with appropriate response codes:
SystemErrorfor encoding/parsing issuesNoPermissionfor blacklisted configurationsSuccessfor valid updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify configuration update implementation coverage
# Check for test coverage of configuration updates
rg -A 5 "fn.*update.*config.*test"
# Check for error handling patterns in the implementation
rg -A 5 "RemotingSysResponseCode::.*Error"
# Look for similar configuration update implementations for reference
ast-grep --pattern 'fn $update_name($_, $_) {
$$$
RemotingSysResponseCode::$_
$$$
}'
Length of output: 17068
| fn update_config(&mut self, request: RemotingCommand) -> RemotingCommand { | ||
| if let Some(body) = request.body() { | ||
| let body_str = match str::from_utf8(&body) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| return RemotingCommand::create_response_command_with_code( | ||
| RemotingSysResponseCode::SystemError, | ||
| ) | ||
| .set_remark(format!("UnsupportedEncodingException {:?}", e)); | ||
| } | ||
| }; | ||
|
|
||
| let properties = match string_to_properties(body_str) { | ||
| Some(props) => props, | ||
| None => { | ||
| return RemotingCommand::create_response_command_with_code( | ||
| RemotingSysResponseCode::SystemError, | ||
| ) | ||
| .set_remark("string_to_properties error".to_string()); | ||
| } | ||
| }; | ||
| if validate_blacklist_config_exist( | ||
| &properties, | ||
| &self | ||
| .kvconfig_manager | ||
| .get_namesrv_config() | ||
| .get_config_blacklist(), | ||
| ) { | ||
| return RemotingCommand::create_response_command_with_code( | ||
| RemotingSysResponseCode::NoPermission, | ||
| ) | ||
| .set_remark("Cannot update config in blacklist.".to_string()); | ||
| } | ||
|
|
||
| let result = self.kvconfig_manager.update_namesrv_config(properties); | ||
| if let Err(e) = result { | ||
| return RemotingCommand::create_response_command_with_code( | ||
| RemotingSysResponseCode::SystemError, | ||
| ) | ||
| .set_remark(format!("Update error {:?}", e)); | ||
| } | ||
| } | ||
|
|
||
| RemotingCommand::create_response_command_with_code(RemotingSysResponseCode::Success) | ||
| .set_remark(CheetahString::empty()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and response generation
The implementation could be enhanced in several ways:
- Error messages should be more descriptive for better debugging
- Empty body case should be handled explicitly
- Success response doesn't need an empty remark
- Error handling could be more idiomatic using Rust's
?operator
Consider this refactoring:
fn update_config(&mut self, request: RemotingCommand) -> RemotingCommand {
+ let body = match request.body() {
+ Some(body) if !body.is_empty() => body,
+ _ => return RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::SystemError
+ ).set_remark("Empty request body"),
+ };
+
+ let body_str = match str::from_utf8(&body) {
+ Ok(s) => s,
+ Err(e) => return RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::SystemError
+ ).set_remark(format!("Invalid UTF-8 in request body: {}", e)),
+ };
+
+ let properties = match string_to_properties(body_str) {
+ Some(props) => props,
+ None => return RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::SystemError
+ ).set_remark("Failed to parse properties from request body"),
+ };
+
+ if validate_blacklist_config_exist(
+ &properties,
+ &self.kvconfig_manager.get_namesrv_config().get_config_blacklist(),
+ ) {
+ return RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::NoPermission
+ ).set_remark("Configuration update blocked by blacklist");
+ }
+
+ match self.kvconfig_manager.update_namesrv_config(properties) {
+ Ok(_) => RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::Success
+ ),
+ Err(e) => RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::SystemError
+ ).set_remark(format!("Failed to update configuration: {}", e)),
+ }
}📝 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 update_config(&mut self, request: RemotingCommand) -> RemotingCommand { | |
| if let Some(body) = request.body() { | |
| let body_str = match str::from_utf8(&body) { | |
| Ok(s) => s, | |
| Err(e) => { | |
| return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError, | |
| ) | |
| .set_remark(format!("UnsupportedEncodingException {:?}", e)); | |
| } | |
| }; | |
| let properties = match string_to_properties(body_str) { | |
| Some(props) => props, | |
| None => { | |
| return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError, | |
| ) | |
| .set_remark("string_to_properties error".to_string()); | |
| } | |
| }; | |
| if validate_blacklist_config_exist( | |
| &properties, | |
| &self | |
| .kvconfig_manager | |
| .get_namesrv_config() | |
| .get_config_blacklist(), | |
| ) { | |
| return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::NoPermission, | |
| ) | |
| .set_remark("Cannot update config in blacklist.".to_string()); | |
| } | |
| let result = self.kvconfig_manager.update_namesrv_config(properties); | |
| if let Err(e) = result { | |
| return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError, | |
| ) | |
| .set_remark(format!("Update error {:?}", e)); | |
| } | |
| } | |
| RemotingCommand::create_response_command_with_code(RemotingSysResponseCode::Success) | |
| .set_remark(CheetahString::empty()) | |
| } | |
| fn update_config(&mut self, request: RemotingCommand) -> RemotingCommand { | |
| let body = match request.body() { | |
| Some(body) if !body.is_empty() => body, | |
| _ => return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError | |
| ).set_remark("Empty request body"), | |
| }; | |
| let body_str = match str::from_utf8(&body) { | |
| Ok(s) => s, | |
| Err(e) => return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError | |
| ).set_remark(format!("Invalid UTF-8 in request body: {}", e)), | |
| }; | |
| let properties = match string_to_properties(body_str) { | |
| Some(props) => props, | |
| None => return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError | |
| ).set_remark("Failed to parse properties from request body"), | |
| }; | |
| if validate_blacklist_config_exist( | |
| &properties, | |
| &self.kvconfig_manager.get_namesrv_config().get_config_blacklist(), | |
| ) { | |
| return RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::NoPermission | |
| ).set_remark("Configuration update blocked by blacklist"); | |
| } | |
| match self.kvconfig_manager.update_namesrv_config(properties) { | |
| Ok(_) => RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::Success | |
| ), | |
| Err(e) => RemotingCommand::create_response_command_with_code( | |
| RemotingSysResponseCode::SystemError | |
| ).set_remark(format!("Failed to update configuration: {}", e)), | |
| } | |
| } |
|
🔊@PanGan21 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
rocketmq-namesrv/src/processor/default_request_processor.rs (2)
444-489: Consider improving error handling and response generation.The implementation could be enhanced in several ways:
- Empty body case should be handled explicitly
- Error messages could be more descriptive
- Success response doesn't need an empty remark
Consider this refactoring:
fn update_config(&mut self, request: RemotingCommand) -> RemotingCommand { + let body = match request.body() { + Some(body) if !body.is_empty() => body, + _ => return RemotingCommand::create_response_command_with_code( + RemotingSysResponseCode::SystemError + ).set_remark("Empty request body"), + }; + + let body_str = match str::from_utf8(body) { + Ok(s) => s, + Err(e) => return RemotingCommand::create_response_command_with_code( + RemotingSysResponseCode::SystemError + ).set_remark(format!("Invalid UTF-8 in request body: {}", e)), + }; + + let properties = match string_to_properties(body_str) { + Some(props) => props, + None => return RemotingCommand::create_response_command_with_code( + RemotingSysResponseCode::SystemError + ).set_remark("Failed to parse properties from request body"), + }; + + if validate_blacklist_config_exist( + &properties, + &self.kvconfig_manager.get_namesrv_config().get_config_blacklist(), + ) { + return RemotingCommand::create_response_command_with_code( + RemotingSysResponseCode::NoPermission + ).set_remark("Configuration update blocked by blacklist"); + } + + match self.kvconfig_manager.update_namesrv_config(properties) { + Ok(_) => RemotingCommand::create_response_command_with_code( + RemotingSysResponseCode::Success + ), + Err(e) => RemotingCommand::create_response_command_with_code( + RemotingSysResponseCode::SystemError + ).set_remark(format!("Failed to update configuration: {}", e)), + } }
541-551: Consider optimizing the blacklist validation.The current implementation iterates through the blacklist checking each key. For large blacklists, this could be inefficient.
Consider using a HashSet for O(1) lookups:
fn validate_blacklist_config_exist( properties: &HashMap<CheetahString, CheetahString>, config_blacklist: &[CheetahString], ) -> bool { + use std::collections::HashSet; + let blacklist_set: HashSet<_> = config_blacklist.iter().collect(); + properties.keys().any(|key| blacklist_set.contains(key)) - for black_config in config_blacklist { - if properties.contains_key(black_config.as_str()) { - return true; - } - } - false }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
rocketmq-namesrv/src/processor/default_request_processor.rs(4 hunks)
🔇 Additional comments (2)
rocketmq-namesrv/src/processor/default_request_processor.rs (2)
18-19: LGTM: Import statements are correctly placed and necessary.
The new imports for str and HashMap are required for the new functionality and follow Rust's convention of grouping standard library imports.
108-108: LGTM: RequestCode handler is properly integrated.
The new UpdateNamesrvConfig case is correctly added to the match expression and calls the appropriate handler method.
PanGan21
changed the title
mxsm
changed the title
Which Issue(s) This PR Fixes(Closes)
Fixes #90
Brief Description
Adds support in Namesrv to update namesrv config
How Did You Test This Change?
Unit tests
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests