[ISSUE #4016]♻️Refactor: Simplify kv_config_path and config_store_path methods for improved readability #4017
Conversation
…h methods for improved readability
WalkthroughRefactors path construction in namesrv_config.rs: both kv_config_path() and config_store_path() now use PathBuf with push("namesrv") and updated filenames. Replaces unwrap() calls with unwrap_or_default() for home_dir and to_str conversions. Directory and filenames differ from previous values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@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
There was a problem hiding this comment.
Pull Request Overview
Refactors the path construction methods in namesrv_config.rs to improve code readability by replacing string formatting with Path API methods.
- Simplified
kv_config_path()andconfig_store_path()methods using PathBuf operations instead of manual string concatenation - Enhanced error handling by using
unwrap_or_default()for safer path operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| kv_config_path.push("namesrv"); | ||
| kv_config_path.push("kvConfig.json"); | ||
| kv_config_path.to_str().unwrap_or_default().to_string() | ||
| } | ||
|
|
||
| pub fn config_store_path() -> String { | ||
| format!( | ||
| "{}{}{}{}{}", | ||
| dirs::home_dir().unwrap().to_str().unwrap(), | ||
| MAIN_SEPARATOR, | ||
| "rocketmq-namesrv", | ||
| MAIN_SEPARATOR, | ||
| "rocketmq-namesrv.properties" | ||
| ) | ||
| let mut kv_config_path = dirs::home_dir().unwrap_or_default(); | ||
| kv_config_path.push("namesrv"); |
There was a problem hiding this comment.
The directory name changed from 'rocketmq-namesrv' to 'namesrv', which is a breaking change that will cause the application to look for configuration files in a different location.
Copilot uses AI. Check for mistakes.
| ) | ||
| let mut kv_config_path = dirs::home_dir().unwrap_or_default(); | ||
| kv_config_path.push("namesrv"); | ||
| kv_config_path.push("namesrv.properties"); |
There was a problem hiding this comment.
The file name changed from 'rocketmq-namesrv.properties' to 'namesrv.properties', which is a breaking change that will cause the application to look for a different configuration file.
| kv_config_path.push("namesrv.properties"); | |
| kv_config_path.push("rocketmq-namesrv.properties"); |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4017 +/- ##
==========================================
- Coverage 26.58% 26.58% -0.01%
==========================================
Files 565 565
Lines 80703 80705 +2
==========================================
Hits 21453 21453
- Misses 59250 59252 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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: 3
🧹 Nitpick comments (2)
rocketmq-common/src/common/namesrv/namesrv_config.rs (2)
20-20: Remove unused import
use std::path::MAIN_SEPARATOR;is unused (the code referencesstd::path::MAIN_SEPARATORdirectly later). Drop the import to avoid warnings.-use std::path::MAIN_SEPARATOR;
198-219: Optional: unify Default impl with PathBufTo keep style consistent and avoid manual separators, consider refactoring the Default impl paths to use PathBuf (or the
default_namesrv_file()helper), preserving the same folder/filenames.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-common/src/common/namesrv/namesrv_config.rs(1 hunks)
⏰ 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). (9)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: auto-approve
| pub fn kv_config_path() -> String { | ||
| format!( | ||
| "{}{}{}{}{}", | ||
| dirs::home_dir().unwrap().to_str().unwrap(), | ||
| MAIN_SEPARATOR, | ||
| "rocketmq-namesrv", | ||
| MAIN_SEPARATOR, | ||
| "kvConfig.json" | ||
| ) | ||
| let mut kv_config_path = dirs::home_dir().unwrap_or_default(); | ||
| kv_config_path.push("namesrv"); | ||
| kv_config_path.push("kvConfig.json"); | ||
| kv_config_path.to_str().unwrap_or_default().to_string() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unintended behavior change + silent fallback: align path and avoid empty-string defaults
- Directory changed from "rocketmq-namesrv" to "namesrv" and string conversion now silently returns "" on failure. This diverges from Default::default() (which still uses "rocketmq-namesrv") and risks subtle misconfigurations.
- Keep PathBuf (readable), but preserve prior semantics and consistency with Default/tests.
Apply:
- let mut kv_config_path = dirs::home_dir().unwrap_or_default();
- kv_config_path.push("namesrv");
- kv_config_path.push("kvConfig.json");
- kv_config_path.to_str().unwrap_or_default().to_string()
+ let mut path = dirs::home_dir().expect("home_dir unavailable");
+ path.push("rocketmq-namesrv");
+ path.push("kvConfig.json");
+ path.to_string_lossy().into_owned()Optional helper to DRY both functions (place inside defaults module, outside this hunk):
fn default_namesrv_file(file: &str) -> String {
let mut p = dirs::home_dir().expect("home_dir unavailable");
p.push("rocketmq-namesrv");
p.push(file);
p.to_string_lossy().into_owned()
}🤖 Prompt for AI Agents
In rocketmq-common/src/common/namesrv/namesrv_config.rs around lines 39 to 44,
restore the original semantics by keeping a PathBuf and using the
"rocketmq-namesrv" directory (not "namesrv"), avoid silently returning an empty
string on to_str failure, and surface errors instead of falling back: build the
path from dirs::home_dir().expect("home_dir unavailable"), push
"rocketmq-namesrv" and the filename, then convert to a String with
to_string_lossy().into_owned() (or use the provided default_namesrv_file helper
in the defaults module and call it here) so behavior matches Default/tests and
avoids empty-string fallbacks.
💡 Verification agent
🧩 Analysis chain
Repo-wide check for path changes
Scan for usages that assume "rocketmq-namesrv" or "rocketmq-namesrv.properties" to ensure no hidden breakages if names were to change.
Also applies to: 46-51
🏁 Script executed:
#!/bin/bash
# Find hardcoded paths and filenames that could be impacted.
rg -n -C2 -S $'rocketmq-namesrv\\.properties|namesrv\\.properties|rocketmq-namesrv[/\\\\]|[/\\\\]namesrv[/\\\\]' -g '!**/target/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Top-level listing:"
ls -la | sed -n '1,200p' || true
echo
echo "Checking ripgrep (rg) availability..."
if command -v rg >/dev/null 2>&1; then
rg --version || true
echo
echo "Searching repository for 'namesrv' and related filenames (no exclusion globs)..."
rg -n -C2 -S 'rocketmq-namesrv\.properties|namesrv\.properties|rocketmq-namesrv[/\\]|[/\\]namesrv[/\\]|\bnamesrv\b|rocketmq-namesrv' --hidden -uu || true
else
echo "rg not found, falling back to grep"
grep -R -n --exclude-dir=target -E 'rocketmq-namesrv\.properties|namesrv\.properties|rocketmq-namesrv[/\\]|[/\\]namesrv[/\\]|\bnamesrv\b|rocketmq-namesrv' . || true
fi
echo
echo "Show target file if present: rocketmq-common/src/common/namesrv/namesrv_config.rs"
if [ -f rocketmq-common/src/common/namesrv/namesrv_config.rs ]; then
echo "---- file content start ----"
sed -n '1,240p' rocketmq-common/src/common/namesrv/namesrv_config.rs || true
echo "---- file content end ----"
else
echo "File not found at rocketmq-common/src/common/namesrv/namesrv_config.rs"
fiLength of output: 73664
Unify namesrv path naming: resolve 'namesrv' vs 'rocketmq-namesrv' inconsistency
defaults::kv_config_path / defaults::config_store_path build paths under ~/namesrv/* while Default::default and format! calls build ~/rocketmq-namesrv/* — this will cause inconsistent runtime locations and break config/kv loading. Pick one canonical directory name (or add explicit fallback checks) and update all occurrences.
Locations to fix:
- rocketmq-common/src/common/namesrv/namesrv_config.rs — defaults::kv_config_path (lines ~39–43) and defaults::config_store_path (46–50); Default::default uses "rocketmq-namesrv" (lines ~204–217) and format! uses "rocketmq-namesrv" (lines ~483–496).
- distribution/config/nameserver/namesrv.toml and distribution/config/nameserver/namesrv.conf — kvConfigPath/configStorePath point to ${user.home}/namesrv/... (lines ~20–24).
🤖 Prompt for AI Agents
In rocketmq-common/src/common/namesrv/namesrv_config.rs around lines 39–44, the
kv_config_path currently builds a path under ~/namesrv/* which conflicts with
other code that uses ~/rocketmq-namesrv/*; change the directory component from
"namesrv" to "rocketmq-namesrv" (and apply the same change for config_store_path
near lines 46–50) so all defaults use a single canonical directory name, or
alternatively implement a fallback check that prefers ~/rocketmq-namesrv/* and
falls back to ~/namesrv/* when files are missing; update any hardcoded format!
or Default::default occurrences to the same canonical name to keep runtime
locations consistent.
| pub fn config_store_path() -> String { | ||
| format!( | ||
| "{}{}{}{}{}", | ||
| dirs::home_dir().unwrap().to_str().unwrap(), | ||
| MAIN_SEPARATOR, | ||
| "rocketmq-namesrv", | ||
| MAIN_SEPARATOR, | ||
| "rocketmq-namesrv.properties" | ||
| ) | ||
| let mut kv_config_path = dirs::home_dir().unwrap_or_default(); | ||
| kv_config_path.push("namesrv"); | ||
| kv_config_path.push("namesrv.properties"); | ||
| kv_config_path.to_str().unwrap_or_default().to_string() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Filename and folder regressions + misnamed local var
- Folder changed to "namesrv" and file to "namesrv.properties" (was "rocketmq-namesrv.properties"), which breaks backward compatibility and differs from Default::default().
- Local var
kv_config_pathis misleading inconfig_store_path().
Apply:
- let mut kv_config_path = dirs::home_dir().unwrap_or_default();
- kv_config_path.push("namesrv");
- kv_config_path.push("namesrv.properties");
- kv_config_path.to_str().unwrap_or_default().to_string()
+ let mut path = dirs::home_dir().expect("home_dir unavailable");
+ path.push("rocketmq-namesrv");
+ path.push("rocketmq-namesrv.properties");
+ path.to_string_lossy().into_owned()If you add the helper suggested above:
- let mut path = dirs::home_dir().expect("home_dir unavailable");
- path.push("rocketmq-namesrv");
- path.push("rocketmq-namesrv.properties");
- path.to_string_lossy().into_owned()
+ default_namesrv_file("rocketmq-namesrv.properties")📝 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.
| pub fn config_store_path() -> String { | |
| format!( | |
| "{}{}{}{}{}", | |
| dirs::home_dir().unwrap().to_str().unwrap(), | |
| MAIN_SEPARATOR, | |
| "rocketmq-namesrv", | |
| MAIN_SEPARATOR, | |
| "rocketmq-namesrv.properties" | |
| ) | |
| let mut kv_config_path = dirs::home_dir().unwrap_or_default(); | |
| kv_config_path.push("namesrv"); | |
| kv_config_path.push("namesrv.properties"); | |
| kv_config_path.to_str().unwrap_or_default().to_string() | |
| } | |
| pub fn config_store_path() -> String { | |
| - let mut kv_config_path = dirs::home_dir().unwrap_or_default(); | |
| - kv_config_path.push("namesrv"); | |
| - kv_config_path.push("namesrv.properties"); | |
| let mut path = dirs::home_dir().expect("home_dir unavailable"); | |
| path.push("rocketmq-namesrv"); | |
| path.push("rocketmq-namesrv.properties"); | |
| path.to_string_lossy().into_owned() | |
| } |
🤖 Prompt for AI Agents
In rocketmq-common/src/common/namesrv/namesrv_config.rs around lines 46 to 51,
the function changed the folder/file to "namesrv/namesrv.properties" and uses a
misleading local variable name `kv_config_path`; restore backward compatibility
by using the original filename "rocketmq-namesrv.properties" (and the original
folder if applicable to match Default::default()), and rename the local variable
to a clearer name such as `config_path` or `path`; if you added the suggested
helper, call it here instead of duplicating logic so the path generation is
centralized and consistent with Default::default().
3 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4016
Brief Description
How Did You Test This Change?
Summary by CodeRabbit