[ISSUE #5555]✨Enhance broker startup process with improved argument validation and configuration handling #5556
Conversation
…alidation and configuration handling
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
WalkthroughReworks broker startup into a multi-step initialization: logger/logo, ROCKETMQ_HOME verification, prioritized config file parsing, system-property extraction/application, command-line overrides, validation of broker/namesrv settings, optional config printing, and then broker boot with explicit error handling and exit codes. Changes
Sequence Diagram(s)sequenceDiagram
%% Participants
participant User
participant CLI as CLI Parser (Args)
participant Env as Environment (ROCKETMQ_HOME)
participant FS as Filesystem/Config
participant Props as System Properties
participant Validator as Validator
participant Broker as Broker Boot
User->>CLI: invoke broker with args
CLI->>CLI: parse args
CLI->>Env: verify ROCKETMQ_HOME
alt ROCKETMQ_HOME invalid
Env-->>CLI: error
CLI-->>User: exit with failure
else ROCKETMQ_HOME valid
CLI->>FS: locate & parse config file (priority: -c then default)
FS-->>CLI: BrokerConfig + MessageStoreConfig
CLI->>Props: extract_properties_from_config
Props-->>CLI: properties map
CLI->>CLI: apply_system_properties & apply_command_line_args
CLI->>Validator: validate_namesrv_address & validate_broker_config
alt validation fails
Validator-->>CLI: error
CLI-->>User: exit with failure
else validation succeeds
alt print requested
CLI->>User: print config (important/all) and exit
else
CLI->>CLI: print_startup_info
CLI->>Broker: boot broker with final configs
Broker-->>User: broker running
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
|
🔊@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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @rocketmq-broker/src/bin/broker_bootstrap_server.rs:
- Around line 173-189: The branch incorrectly treats an existing-but-empty
NAMESRV_ADDR env var as a valid override because it uses
env::var("NAMESRV_ADDR").is_ok(); change the condition so it only triggers when
the env var is non-empty (e.g. replace env::var(...).is_ok() with
env::var("NAMESRV_ADDR").ok().filter(|s| !s.is_empty()).is_some()), keep the
existing args.namesrv_addr check and continue using args.get_namesrv_addr() to
populate broker_config.namesrv_addr and logging via
broker_config.namesrv_addr.as_ref().unwrap().
In @rocketmq-broker/src/command.rs:
- Around line 256-271: The four calls to env::set_var inside
apply_system_properties need to be wrapped in unsafe blocks because env::set_var
is an unsafe fn in Rust 1.85.0; update each call (both the domain and subgroup
branches where env::set_var("rocketmq.namesrv.domain", ...),
env::set_var("rocketmq.namesrv.domain", DEFAULT_DOMAIN),
env::set_var("rocketmq.namesrv.domain.subgroup", ...), and
env::set_var("rocketmq.namesrv.domain.subgroup", DEFAULT_SUBGROUP)) to be
invoked as unsafe { env::set_var(...) } so the function compiles under the
declared MSRV while preserving the existing logic and single-threaded
initialization assumption.
🧹 Nitpick comments (5)
rocketmq-broker/src/command.rs (1)
125-143: Consider separating logging from pure validation.The
validate()method performs logging (info!) as a side effect. This can be unexpected behavior for a validation function, which is typically expected to be pure. Logging could be moved to the caller after successful validation.rocketmq-broker/src/bin/broker_bootstrap_server.rs (4)
59-98: Consider returning errors instead of usingprocess::exit.Using
process::exit()bypasses Rust's normal cleanup mechanisms (destructors won't run). Sincemain()already returnsResult<()>, consider propagating errors instead. This would be more idiomatic and allow proper resource cleanup.♻️ Proposed refactor sketch
- if let Err(e) = args.validate() { - error!("Invalid arguments: {}", e); - process::exit(-1); - } + args.validate().map_err(|e| { + error!("Invalid arguments: {}", e); + anyhow::anyhow!("Invalid arguments: {}", e) + })?;Apply similar pattern to other
process::exitcalls, usinganyhow::bail!or?operator to propagate errors.
155-165: Placeholder implementation - consider marking as TODO or implementing.This function currently returns an empty
HashMap, makingapply_system_propertiesalways use default values. If this is intentional for now, consider adding a TODO comment or removing the call until it's properly implemented.
211-234: Duplicate validation logic with different strictness.
validate_namesrv_addresshere only checks for a colon separator, whileArgs::validate_namesrv_addrincommand.rsperforms stricter validation (SocketAddr parsing, port number validation). This inconsistency could allow invalid addresses through if they bypass CLI validation. Consider reusing the stricter validation or consolidating the logic.
284-294: Incomplete implementation forMessageStoreConfigprinting.The comment notes that
MessageStoreConfigdoesn't implementget_propertiesyet. This results in the "all config" output being less complete than "important config" for message store settings. Consider adding a TODO or implementingget_propertiesfor consistency.Would you like me to help create an issue to track implementing
get_propertiesforMessageStoreConfig?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-broker/src/bin/broker_bootstrap_server.rsrocketmq-broker/src/command.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-broker/src/command.rs (1)
rocketmq-common/src/common/namesrv/namesrv_config.rs (1)
rocketmq_home(31-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
🔇 Additional comments (8)
rocketmq-broker/src/command.rs (4)
25-51: LGTM! Well-structured error enum.The
BrokerArgsErrorenum provides clear, descriptive error messages with proper use ofthiserror. The error variants cover the expected failure modes for argument validation.
60-112: LGTM! Clean clap configuration with appropriate constraints.The
conflicts_withattribute correctly ensures mutual exclusivity between-pand-mflags. The documentation clearly explains the precedence and format expectations.
205-223: LGTM! Clear priority chain for nameserver address resolution.The priority (CLI → environment → default) is well-documented and correctly implemented.
274-293: LGTM!The
Displayimplementation provides a clean, readable format for debugging and logging purposes.rocketmq-broker/src/bin/broker_bootstrap_server.rs (4)
123-126: ROCKETMQ_HOME validation inconsistency.If
ROCKETMQ_HOMEis set but points to a non-existent or non-directory path, only a warning is logged and startup continues. This could lead to confusing errors later (e.g., when trying to load config from$ROCKETMQ_HOME/conf/broker.toml). Consider failing early or documenting this as intentional behavior.
138-153: LGTM! Config parsing with appropriate error context.The use of
with_contextprovides helpful error messages when config parsing fails. The fallback to defaults when no config file exists is sensible.
296-313: LGTM!Clear and informative startup logging that helps with debugging and operational visibility.
315-338: LGTM!Unit tests provide good coverage of the nameserver address validation edge cases.
| if args.namesrv_addr.is_some() || env::var("NAMESRV_ADDR").is_ok() { | ||
| let namesrv_addr = args.get_namesrv_addr(); | ||
| broker_config.namesrv_addr = Some(namesrv_addr.into()); | ||
| info!( | ||
| "Name server address (from command line/env): {}", | ||
| broker_config.namesrv_addr.as_ref().unwrap() | ||
| ); | ||
| } else if let Some(ref addr) = broker_config.namesrv_addr { | ||
| info!("Name server address (from config file): {}", addr); | ||
| } else { | ||
| // Use default if not set anywhere | ||
| broker_config.namesrv_addr = Some("127.0.0.1:9876".to_string().into()); | ||
| info!( | ||
| "Name server address (default): {}", | ||
| broker_config.namesrv_addr.as_ref().unwrap() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Empty NAMESRV_ADDR env var triggers override path unexpectedly.
env::var("NAMESRV_ADDR").is_ok() returns true if the variable exists but is empty. This causes the code to enter the CLI/env override branch, though get_namesrv_addr() would then return the default. Consider checking for non-empty explicitly to match the behavior in get_namesrv_addr().
🔧 Proposed fix
- if args.namesrv_addr.is_some() || env::var("NAMESRV_ADDR").is_ok() {
+ if args.namesrv_addr.is_some() || env::var("NAMESRV_ADDR").map(|v| !v.is_empty()).unwrap_or(false) {📝 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.
| if args.namesrv_addr.is_some() || env::var("NAMESRV_ADDR").is_ok() { | |
| let namesrv_addr = args.get_namesrv_addr(); | |
| broker_config.namesrv_addr = Some(namesrv_addr.into()); | |
| info!( | |
| "Name server address (from command line/env): {}", | |
| broker_config.namesrv_addr.as_ref().unwrap() | |
| ); | |
| } else if let Some(ref addr) = broker_config.namesrv_addr { | |
| info!("Name server address (from config file): {}", addr); | |
| } else { | |
| // Use default if not set anywhere | |
| broker_config.namesrv_addr = Some("127.0.0.1:9876".to_string().into()); | |
| info!( | |
| "Name server address (default): {}", | |
| broker_config.namesrv_addr.as_ref().unwrap() | |
| ); | |
| } | |
| if args.namesrv_addr.is_some() || env::var("NAMESRV_ADDR").map(|v| !v.is_empty()).unwrap_or(false) { | |
| let namesrv_addr = args.get_namesrv_addr(); | |
| broker_config.namesrv_addr = Some(namesrv_addr.into()); | |
| info!( | |
| "Name server address (from command line/env): {}", | |
| broker_config.namesrv_addr.as_ref().unwrap() | |
| ); | |
| } else if let Some(ref addr) = broker_config.namesrv_addr { | |
| info!("Name server address (from config file): {}", addr); | |
| } else { | |
| // Use default if not set anywhere | |
| broker_config.namesrv_addr = Some("127.0.0.1:9876".to_string().into()); | |
| info!( | |
| "Name server address (default): {}", | |
| broker_config.namesrv_addr.as_ref().unwrap() | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @rocketmq-broker/src/bin/broker_bootstrap_server.rs around lines 173 - 189,
The branch incorrectly treats an existing-but-empty NAMESRV_ADDR env var as a
valid override because it uses env::var("NAMESRV_ADDR").is_ok(); change the
condition so it only triggers when the env var is non-empty (e.g. replace
env::var(...).is_ok() with env::var("NAMESRV_ADDR").ok().filter(|s|
!s.is_empty()).is_some()), keep the existing args.namesrv_addr check and
continue using args.get_namesrv_addr() to populate broker_config.namesrv_addr
and logging via broker_config.namesrv_addr.as_ref().unwrap().
| /// Check if a string is valid host:port format | ||
| fn is_valid_host_port(&self, addr: &str) -> bool { | ||
| let parts: Vec<&str> = addr.split(':').collect(); | ||
| if parts.len() != 2 { | ||
| return false; | ||
| } | ||
|
|
||
| // Validate port is a number | ||
| parts[1].parse::<u16>().is_ok() | ||
| } |
There was a problem hiding this comment.
Host validation is insufficient.
is_valid_host_port only validates that the port is a valid u16 but doesn't validate the host portion. Addresses like :9876, !@#$:9876, or empty hosts would pass validation.
🔧 Proposed fix to add minimal host validation
fn is_valid_host_port(&self, addr: &str) -> bool {
let parts: Vec<&str> = addr.split(':').collect();
if parts.len() != 2 {
return false;
}
+ // Validate host is not empty
+ let host = parts[0].trim();
+ if host.is_empty() {
+ return false;
+ }
+
// Validate port is a number
parts[1].parse::<u16>().is_ok()
}📝 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.
| /// Check if a string is valid host:port format | |
| fn is_valid_host_port(&self, addr: &str) -> bool { | |
| let parts: Vec<&str> = addr.split(':').collect(); | |
| if parts.len() != 2 { | |
| return false; | |
| } | |
| // Validate port is a number | |
| parts[1].parse::<u16>().is_ok() | |
| } | |
| /// Check if a string is valid host:port format | |
| fn is_valid_host_port(&self, addr: &str) -> bool { | |
| let parts: Vec<&str> = addr.split(':').collect(); | |
| if parts.len() != 2 { | |
| return false; | |
| } | |
| // Validate host is not empty | |
| let host = parts[0].trim(); | |
| if host.is_empty() { | |
| return false; | |
| } | |
| // Validate port is a number | |
| parts[1].parse::<u16>().is_ok() | |
| } |
| pub fn apply_system_properties(properties: &HashMap<String, String>) { | ||
| const DEFAULT_DOMAIN: &str = "jmenv.tbsite.net"; | ||
| const DEFAULT_SUBGROUP: &str = "nsaddr"; | ||
|
|
||
| if let Some(domain) = properties.get("rmqAddressServerDomain") { | ||
| env::set_var("rocketmq.namesrv.domain", domain); | ||
| } else { | ||
| env::set_var("rocketmq.namesrv.domain", DEFAULT_DOMAIN); | ||
| } | ||
|
|
||
| if let Some(subgroup) = properties.get("rmqAddressServerSubGroup") { | ||
| env::set_var("rocketmq.namesrv.domain.subgroup", subgroup); | ||
| } else { | ||
| env::set_var("rocketmq.namesrv.domain.subgroup", DEFAULT_SUBGROUP); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
env::set_var thread safety Rust 1.80 unsafe deprecated
💡 Result:
Short answer: In modern Rust (including 1.80) std::env::set_var is marked unsafe because it is not safe to call in general multi-threaded programs on Unix-like platforms (Windows is safe). You must call it inside an unsafe block and ensure no other threads may read or write the process environment; prefer Command::env for child processes. [1][2][3]
Notes/sources:
- std::env::set_var docs (safety text: safe only single‑threaded except on Windows). [1]
- Rust 2024/edition guidance: set_var/remove_var were made unsafe (migration guidance). [2]
- Historical discussion about deprecating/changing set_var (context on thread-safety concerns). [3]
🌐 Web query:
Rust std::env::set_var safety documentation changelog
💡 Result:
Short summary
-
std::env::set_var was made unsafe in the Rust 2024 edition; the standard-library docs now show its signature as
pub unsafe fn set_var(...)and include the safety text about single-threaded/Windows-only safety (Unix multi-threaded use is unsound). [1] -
The change is documented in the Rust 2024 edition guide under "Newly unsafe functions" (lists std::env::set_var and std::env::remove_var). [2]
-
The Rust release announcement / changelog for the stable release that stabilizes the 2024 edition also lists these functions as newly unsafe. [3]
-
Related compiler support: the deprecated-safe-2024 lint flags uses of functions that were safe before but are unsafe in Rust 2024. [4]
Sources
[1] std::env::set_var docs.
[2] Rust Edition Guide — Newly unsafe functions.
[3] Rust blog release notes (Rust 1.85.0 / Rust 2024 announcement).
[4] DEPRECATED_SAFE_2024 lint documentation.
🏁 Script executed:
cd rocketmq-broker && cat -n src/command.rs | sed -n '250,275p'Repository: mxsm/rocketmq-rust
Length of output: 1264
🏁 Script executed:
cd rocketmq-broker && cat Cargo.toml | grep -A5 "edition\|rust-version"Repository: mxsm/rocketmq-rust
Length of output: 431
🏁 Script executed:
cd rocketmq-broker && rg "apply_system_properties" -B5 -A15Repository: mxsm/rocketmq-rust
Length of output: 2768
🏁 Script executed:
cat -n Cargo.toml | head -30Repository: mxsm/rocketmq-rust
Length of output: 907
🏁 Script executed:
rg "edition|rust-version" Cargo.toml | head -20Repository: mxsm/rocketmq-rust
Length of output: 102
Wrap env::set_var calls in unsafe blocks — required by Rust 1.85.0+.
env::set_var was made unsafe fn in Rust 1.85.0 (Rust 2024 edition) because it is unsound in multi-threaded contexts on Unix. Your declared MSRV is 1.85.0, so the four env::set_var calls on lines 261, 263, 267, and 269 must each be wrapped in unsafe { } blocks to compile. While this code runs only once during single-threaded initialization, the unsafe block is now mandatory and documents the single-threaded assumption.
🤖 Prompt for AI Agents
In @rocketmq-broker/src/command.rs around lines 256 - 271, The four calls to
env::set_var inside apply_system_properties need to be wrapped in unsafe blocks
because env::set_var is an unsafe fn in Rust 1.85.0; update each call (both the
domain and subgroup branches where env::set_var("rocketmq.namesrv.domain", ...),
env::set_var("rocketmq.namesrv.domain", DEFAULT_DOMAIN),
env::set_var("rocketmq.namesrv.domain.subgroup", ...), and
env::set_var("rocketmq.namesrv.domain.subgroup", DEFAULT_SUBGROUP)) to be
invoked as unsafe { env::set_var(...) } so the function compiles under the
declared MSRV while preserving the existing logic and single-threaded
initialization assumption.
| #[test] | ||
| fn test_get_namesrv_addr_default() { | ||
| let args = Args { | ||
| config_file: None, | ||
| print_config_item: false, | ||
| print_important_config: false, | ||
| namesrv_addr: None, | ||
| }; | ||
|
|
||
| // Should return default if no env var set | ||
| assert_eq!(args.get_namesrv_addr(), "127.0.0.1:9876"); | ||
| } |
There was a problem hiding this comment.
Test may be flaky if NAMESRV_ADDR environment variable is set.
test_get_namesrv_addr_default assumes no NAMESRV_ADDR environment variable is set, but this may not hold in all test environments. Consider explicitly unsetting the variable before the assertion to ensure test isolation.
🔧 Proposed fix
#[test]
fn test_get_namesrv_addr_default() {
+ // Ensure env var is not set for this test
+ std::env::remove_var("NAMESRV_ADDR");
+
let args = Args {
config_file: None,
print_config_item: false,
print_important_config: false,
namesrv_addr: None,
};
// Should return default if no env var set
assert_eq!(args.get_namesrv_addr(), "127.0.0.1:9876");
}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @rocketmq-broker/src/bin/broker_bootstrap_server.rs:
- Around line 114-130: The is_empty() check in verify_rocketmq_home() is
unreachable because EnvUtils::get_rocketmq_home() always returns a non-empty
fallback; update verify_rocketmq_home to first check the actual environment
variable (e.g., std::env::var or var_os for "ROCKETMQ_HOME") and bail if it is
not set or empty, then call EnvUtils::get_rocketmq_home() to construct the
PathBuf and perform the exists()/is_dir() checks; remove the redundant
home.is_empty() branch so the function enforces an explicit ROCKETMQ_HOME while
preserving the directory validation and logging.
🧹 Nitpick comments (3)
rocketmq-broker/src/bin/broker_bootstrap_server.rs (3)
155-162: Placeholder function returns empty map.This stub always returns an empty
HashMap, soArgs::apply_system_properties()will only apply defaults. This is acceptable if the current typed struct approach is intentional, but consider adding a TODO or removing the function if it's not expected to be enhanced.
208-231: Basic address validation only checks for colon presence.The validation catches malformed addresses without ports but doesn't validate that the port is numeric or within a valid range (1-65535). This is acceptable for a basic check, but consider enhancing if stricter validation is needed.
♻️ Optional enhancement for stricter validation
// Basic validation: should contain colon for port if !trimmed.contains(':') { anyhow::bail!( "Invalid name server address format: {}. Expected format: '127.0.0.1:9876;192.168.0.1:9876'", namesrv_addr ); } + + // Validate port is numeric and in valid range + if let Some(port_str) = trimmed.rsplit(':').next() { + match port_str.parse::<u16>() { + Ok(0) => anyhow::bail!("Invalid port 0 in address: {}", trimmed), + Err(_) => anyhow::bail!("Invalid port '{}' in address: {}", port_str, trimmed), + _ => {} + } + }
312-335: Good test coverage for basic cases.The tests cover single address, multiple addresses, invalid format, and empty input. Consider adding edge case tests for addresses with empty segments (e.g.,
"127.0.0.1:9876;;192.168.0.2:9876") and whitespace-only segments to verify thetrim()andcontinuelogic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-broker/src/bin/broker_bootstrap_server.rsrocketmq-broker/src/command.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rocketmq-broker/src/command.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-broker/src/bin/broker_bootstrap_server.rs (4)
rocketmq-common/src/utils/parse_config_file.rs (2)
parse_config_file(23-32)cfg(30-30)rocketmq-broker/src/command.rs (1)
apply_system_properties(256-271)rocketmq-common/src/utils/env_utils.rs (1)
get_rocketmq_home(120-132)rocketmq-common/src/common/broker/broker_config.rs (2)
broker_identity(59-61)broker_cluster_name(50-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build & Test (macos-latest)
- GitHub Check: Build & Test (windows-latest)
- GitHub Check: Build & Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: auto-approve
🔇 Additional comments (4)
rocketmq-broker/src/bin/broker_bootstrap_server.rs (4)
45-112: Well-structured startup flow with clear error handling.The main function demonstrates good practices: distinct exit codes for different failure modes, clear sequential initialization steps, and proper separation of concerns. The workflow (logger → home verification → config parsing → property extraction → CLI override → validation → optional print → boot) is logical and maintainable.
189-206: Validation is minimal but correct for the current scope.The function currently only validates the name server address format. The docstring mentions "Check broker role configuration" and "Validate broker ID" which aren't fully implemented yet. Consider either implementing these checks or updating the docstring to match the current implementation.
272-291: Asymmetric implementation between config printing functions.
print_all_broker_configusesget_properties()for dynamic enumeration whileprint_all_message_store_configmanually lists fields. The comment at line 284 acknowledges this limitation. This is acceptable, but consider implementingget_properties()forMessageStoreConfigfor consistency if this pattern is used elsewhere.
293-310: Good startup logging for operational visibility.The function provides useful structured logging for broker identity, name server address, and listen endpoint, which is helpful for debugging and monitoring.
| /// Verify ROCKETMQ_HOME environment variable is set | ||
| fn verify_rocketmq_home() -> Result<()> { | ||
| let home = EnvUtils::get_rocketmq_home(); | ||
| info!("Rocketmq(Rust) home: {}", home); | ||
| let config = if let Some(ref config_file) = args.config_file { | ||
| let config_file = PathBuf::from(config_file); | ||
| info!("Using config file: {:?}", config_file); | ||
| if !config_file.exists() || !config_file.is_file() { | ||
| bail!("Config file does not exist or is not a file: {:?}", config_file); | ||
| } | ||
| Ok(( | ||
| ParseConfigFile::parse_config_file::<BrokerConfig>(config_file.clone())?, | ||
| ParseConfigFile::parse_config_file::<MessageStoreConfig>(config_file)?, | ||
| )) | ||
| if home.is_empty() { | ||
| anyhow::bail!( | ||
| "Please set the ROCKETMQ_HOME environment variable to match the location of the RocketMQ installation" | ||
| ); | ||
| } | ||
|
|
||
| let home_path = PathBuf::from(&home); | ||
| if !home_path.exists() || !home_path.is_dir() { | ||
| warn!("ROCKETMQ_HOME directory does not exist or is not a directory: {}", home); | ||
| } | ||
|
|
||
| info!("ROCKETMQ_HOME: {}", home); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
home.is_empty() check is unreachable due to get_rocketmq_home() fallback behavior.
Based on the EnvUtils::get_rocketmq_home() implementation in rocketmq-common/src/utils/env_utils.rs, the function always returns a non-empty string (falling back to "." if ROCKETMQ_HOME is not set). The is_empty() check on line 117 will never be true.
If the intent is to require explicit ROCKETMQ_HOME configuration, consider checking the environment variable directly before calling get_rocketmq_home().
🔧 Proposed fix
fn verify_rocketmq_home() -> Result<()> {
+ // Check if ROCKETMQ_HOME is explicitly set before using get_rocketmq_home()
+ // which has fallback behavior
+ if env::var("ROCKETMQ_HOME").map(|v| v.is_empty()).unwrap_or(true) {
+ warn!("ROCKETMQ_HOME is not set, using current directory as default");
+ }
+
let home = EnvUtils::get_rocketmq_home();
- if home.is_empty() {
- anyhow::bail!(
- "Please set the ROCKETMQ_HOME environment variable to match the location of the RocketMQ installation"
- );
- }
let home_path = PathBuf::from(&home);
if !home_path.exists() || !home_path.is_dir() {📝 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.
| /// Verify ROCKETMQ_HOME environment variable is set | |
| fn verify_rocketmq_home() -> Result<()> { | |
| let home = EnvUtils::get_rocketmq_home(); | |
| info!("Rocketmq(Rust) home: {}", home); | |
| let config = if let Some(ref config_file) = args.config_file { | |
| let config_file = PathBuf::from(config_file); | |
| info!("Using config file: {:?}", config_file); | |
| if !config_file.exists() || !config_file.is_file() { | |
| bail!("Config file does not exist or is not a file: {:?}", config_file); | |
| } | |
| Ok(( | |
| ParseConfigFile::parse_config_file::<BrokerConfig>(config_file.clone())?, | |
| ParseConfigFile::parse_config_file::<MessageStoreConfig>(config_file)?, | |
| )) | |
| if home.is_empty() { | |
| anyhow::bail!( | |
| "Please set the ROCKETMQ_HOME environment variable to match the location of the RocketMQ installation" | |
| ); | |
| } | |
| let home_path = PathBuf::from(&home); | |
| if !home_path.exists() || !home_path.is_dir() { | |
| warn!("ROCKETMQ_HOME directory does not exist or is not a directory: {}", home); | |
| } | |
| info!("ROCKETMQ_HOME: {}", home); | |
| Ok(()) | |
| } | |
| /// Verify ROCKETMQ_HOME environment variable is set | |
| fn verify_rocketmq_home() -> Result<()> { | |
| // Check if ROCKETMQ_HOME is explicitly set before using get_rocketmq_home() | |
| // which has fallback behavior | |
| if env::var("ROCKETMQ_HOME").map(|v| v.is_empty()).unwrap_or(true) { | |
| warn!("ROCKETMQ_HOME is not set, using current directory as default"); | |
| } | |
| let home = EnvUtils::get_rocketmq_home(); | |
| let home_path = PathBuf::from(&home); | |
| if !home_path.exists() || !home_path.is_dir() { | |
| warn!("ROCKETMQ_HOME directory does not exist or is not a directory: {}", home); | |
| } | |
| info!("ROCKETMQ_HOME: {}", home); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In @rocketmq-broker/src/bin/broker_bootstrap_server.rs around lines 114 - 130,
The is_empty() check in verify_rocketmq_home() is unreachable because
EnvUtils::get_rocketmq_home() always returns a non-empty fallback; update
verify_rocketmq_home to first check the actual environment variable (e.g.,
std::env::var or var_os for "ROCKETMQ_HOME") and bail if it is not set or empty,
then call EnvUtils::get_rocketmq_home() to construct the PathBuf and perform the
exists()/is_dir() checks; remove the redundant home.is_empty() branch so the
function enforces an explicit ROCKETMQ_HOME while preserving the directory
validation and logging.
| /// Parse configuration from file | ||
| /// | ||
| /// Priority: | ||
| /// 1. Explicit config file from `-c` argument | ||
| /// 2. $ROCKETMQ_HOME/conf/broker.toml | ||
| /// 3. Default configuration | ||
| fn parse_config_file(args: &Args) -> Result<(BrokerConfig, MessageStoreConfig)> { | ||
| if let Some(config_file) = args.get_config_file() { | ||
| info!("Loading configuration from: {}", config_file.display()); | ||
|
|
||
| let broker_config = ParseConfigFile::parse_config_file::<BrokerConfig>(config_file.clone()) | ||
| .with_context(|| format!("Failed to parse BrokerConfig from {:?}", config_file))?; | ||
|
|
||
| let message_store_config = ParseConfigFile::parse_config_file::<MessageStoreConfig>(config_file.clone()) | ||
| .with_context(|| format!("Failed to parse MessageStoreConfig from {:?}", config_file))?; | ||
|
|
||
| Ok((broker_config, message_store_config)) | ||
| } else { | ||
| let config_file = PathBuf::from(home.as_str()).join("conf").join("broker.toml"); | ||
| info!("Using config file: {:?}", config_file); | ||
| if !config_file.exists() || !config_file.is_file() { | ||
| return Ok((Default::default(), Default::default())); | ||
| info!("Using default configuration (no config file specified)"); | ||
| Ok((BrokerConfig::default(), MessageStoreConfig::default())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation missing documented fallback to $ROCKETMQ_HOME/conf/broker.toml.
The docstring states priority 2 is $ROCKETMQ_HOME/conf/broker.toml, but the implementation only handles the explicit -c argument (priority 1) and default configuration (priority 3). The intermediate fallback path is not implemented.
🔧 Proposed fix to implement the documented behavior
fn parse_config_file(args: &Args) -> Result<(BrokerConfig, MessageStoreConfig)> {
if let Some(config_file) = args.get_config_file() {
info!("Loading configuration from: {}", config_file.display());
let broker_config = ParseConfigFile::parse_config_file::<BrokerConfig>(config_file.clone())
.with_context(|| format!("Failed to parse BrokerConfig from {:?}", config_file))?;
let message_store_config = ParseConfigFile::parse_config_file::<MessageStoreConfig>(config_file.clone())
.with_context(|| format!("Failed to parse MessageStoreConfig from {:?}", config_file))?;
Ok((broker_config, message_store_config))
} else {
+ // Try default config file location
+ let default_config = PathBuf::from(EnvUtils::get_rocketmq_home())
+ .join("conf")
+ .join("broker.toml");
+
+ if default_config.exists() {
+ info!("Loading configuration from default location: {}", default_config.display());
+
+ let broker_config = ParseConfigFile::parse_config_file::<BrokerConfig>(default_config.clone())
+ .with_context(|| format!("Failed to parse BrokerConfig from {:?}", default_config))?;
+
+ let message_store_config = ParseConfigFile::parse_config_file::<MessageStoreConfig>(default_config.clone())
+ .with_context(|| format!("Failed to parse MessageStoreConfig from {:?}", default_config))?;
+
+ return Ok((broker_config, message_store_config));
+ }
+
info!("Using default configuration (no config file specified)");
Ok((BrokerConfig::default(), MessageStoreConfig::default()))
}
}Committable suggestion skipped: line range outside the PR's diff.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5556 +/- ##
==========================================
+ Coverage 38.49% 38.54% +0.04%
==========================================
Files 815 816 +1
Lines 110608 110917 +309
==========================================
+ Hits 42575 42748 +173
- Misses 68033 68169 +136 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #5555
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.