Skip to content

[ISSUE #4016]♻️Refactor: Simplify kv_config_path and config_store_path methods for improved readability #4017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 11, 2025
Merged

[ISSUE #4016]♻️Refactor: Simplify kv_config_path and config_store_path methods for improved readability #4017

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 8 additions & 16 deletions rocketmq-common/src/common/namesrv/namesrv_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,25 +37,17 @@ mod defaults {
}

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()
}
Comment on lines 39 to 44
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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"
fi

Length 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");
Comment on lines +41 to +48
Copy link

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
kv_config_path.push("namesrv.properties");
Copy link

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
kv_config_path.push("namesrv.properties");
kv_config_path.push("rocketmq-namesrv.properties");

Copilot uses AI. Check for mistakes.
kv_config_path.to_str().unwrap_or_default().to_string()
}
Comment on lines 46 to 51
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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_path is misleading in config_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.

Suggested change
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().


pub fn product_env_name() -> String {
Expand Down
Loading