[ISSUE #4020]⚡️Improve name server readiness checks and update log messages for clarity #4021
Conversation
…ssages for clarity
WalkthroughThe name server readiness check now uses explicit elapsed time against a configured wait_seconds and an additional need_wait_for_service flag. Log and error messages were updated for clarity. Topic route encoding always uses standard JSON serialization with direct response body setting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Processor as ClientRequestProcessor
participant Cfg as Config
participant Clock as Time Source
participant NS as Name Server State
Client->>Processor: Request topic route
Processor->>Cfg: Read need_wait_for_service, wait_seconds
Processor->>Clock: Compute elapsed_millis since start
alt need_wait_for_service && elapsed_millis < wait_seconds*1000
Processor-->>Client: SystemError ("name server not ready")
else Ready
Processor->>NS: Fetch topic route data
alt Route not found
Processor-->>Client: Error ("No topic route info in name server for the topic: ...")
else Route found
note right of Processor: Encode via standard JSON serialization
Processor-->>Client: Response with encoded route data
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
✨ 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
This PR improves the name server readiness check implementation and standardizes log messages for better clarity. The changes refactor the readiness check logic to be more readable and consistent while removing commented-out legacy code.
- Refactored readiness check calculation with clearer variable names and safer arithmetic operations
- Updated log messages to consistently use "name server" instead of "remoting_server"
- Removed unused imports and commented-out legacy code
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } else { | ||
| topic_route_data.encode() | ||
| };*/ | ||
| //Rust only support standard JSON serialization |
There was a problem hiding this comment.
Grammar error in comment: should be 'supports' instead of 'support'.
| //Rust only support standard JSON serialization | |
| //Rust only supports standard JSON serialization |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4021 +/- ##
=======================================
Coverage 26.57% 26.57%
=======================================
Files 565 565
Lines 80722 80720 -2
=======================================
Hits 21453 21453
+ Misses 59269 59267 -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: 1
🧹 Nitpick comments (3)
rocketmq-namesrv/src/processor/client_request_processor.rs (3)
96-107: Improve diagnosability of “not ready” pathLog the elapsed vs. configured wait and echo it in the remark; removes trailing space and eases ops triage.
- if need_wait_for_service && !namesrv_ready { - warn!("name server not ready. request code {} ", request.code()); + if need_wait_for_service && !namesrv_ready { + warn!( + "name server not ready: elapsed={}ms < wait={}s (code={})", + elapsed_millis, wait_seconds, request.code() + ); return Ok(Some( RemotingCommand::create_response_command_with_code( RemotingSysResponseCode::SystemError, ) - .set_remark("name server not ready"), + .set_remark(format!( + "name server not ready: elapsed={}ms < wait={}s", + elapsed_millis, wait_seconds + )), )); }
116-120: Tighten TopicNotExist remark wordingMinor clarity/readability tweak; quote the topic and add spacing before FAQ text.
- .set_remark(format!( - "No topic route info in name server for the topic:{}{}", - request_header.topic, - FAQUrl::suggest_todo(FAQUrl::APPLY_TOPIC_URL) - )), + .set_remark(format!( + "No topic route info in name server for topic '{}'. {}", + request_header.topic, + FAQUrl::suggest_todo(FAQUrl::APPLY_TOPIC_URL) + )),
142-142: Fix comment grammar- //Rust only support standard JSON serialization + // Rust only supports standard JSON serialization
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-namesrv/src/processor/client_request_processor.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-namesrv/src/processor/client_request_processor.rs (2)
rocketmq-common/src/utils/time_utils.rs (1)
get_current_millis(19-24)rocketmq-remoting/src/protocol/remoting_command.rs (2)
code(578-580)create_response_command_with_code(192-194)
⏰ 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: build (ubuntu-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: auto-approve
| let wait_seconds = self | ||
| .name_server_runtime_inner | ||
| .name_server_config() | ||
| .wait_seconds_for_service as u64; | ||
| let elapsed_millis = | ||
| TimeUtils::get_current_millis().saturating_sub(self.startup_time_millis); | ||
| let namesrv_ready = self.need_check_namesrv_ready.load(Ordering::Relaxed) | ||
| && TimeUtils::get_current_millis() - self.startup_time_millis | ||
| >= Duration::from_secs( | ||
| self.name_server_runtime_inner | ||
| .name_server_config() | ||
| .wait_seconds_for_service as u64, | ||
| ) | ||
| .as_millis() as u64; | ||
| if self | ||
| && elapsed_millis >= wait_seconds * 1000; | ||
|
|
There was a problem hiding this comment.
Fix readiness logic: bypass when check is disabled; align atomic ordering; avoid overflow
When need_check_namesrv_ready becomes false, namesrv_ready evaluates to false and the server incorrectly returns “not ready” on subsequent requests. Also, use Acquire to pair with the Release store and guard against u64 overflow.
Apply:
- let elapsed_millis =
- TimeUtils::get_current_millis().saturating_sub(self.startup_time_millis);
- let namesrv_ready = self.need_check_namesrv_ready.load(Ordering::Relaxed)
- && elapsed_millis >= wait_seconds * 1000;
+ let elapsed_millis =
+ TimeUtils::get_current_millis().saturating_sub(self.startup_time_millis);
+ let should_check = self
+ .need_check_namesrv_ready
+ .load(Ordering::Acquire);
+ let namesrv_ready =
+ !should_check || elapsed_millis >= wait_seconds.saturating_mul(1000);📝 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.
| let wait_seconds = self | |
| .name_server_runtime_inner | |
| .name_server_config() | |
| .wait_seconds_for_service as u64; | |
| let elapsed_millis = | |
| TimeUtils::get_current_millis().saturating_sub(self.startup_time_millis); | |
| let namesrv_ready = self.need_check_namesrv_ready.load(Ordering::Relaxed) | |
| && TimeUtils::get_current_millis() - self.startup_time_millis | |
| >= Duration::from_secs( | |
| self.name_server_runtime_inner | |
| .name_server_config() | |
| .wait_seconds_for_service as u64, | |
| ) | |
| .as_millis() as u64; | |
| if self | |
| && elapsed_millis >= wait_seconds * 1000; | |
| let wait_seconds = self | |
| .name_server_runtime_inner | |
| .name_server_config() | |
| .wait_seconds_for_service as u64; | |
| let elapsed_millis = | |
| TimeUtils::get_current_millis().saturating_sub(self.startup_time_millis); | |
| let should_check = self | |
| .need_check_namesrv_ready | |
| .load(Ordering::Acquire); | |
| let namesrv_ready = | |
| !should_check || elapsed_millis >= wait_seconds.saturating_mul(1000); |
🤖 Prompt for AI Agents
In rocketmq-namesrv/src/processor/client_request_processor.rs around lines 87 to
95, the readiness check wrongly returns false after need_check_namesrv_ready is
set to false, uses Relaxed ordering, and risks u64 overflow when scaling seconds
to milliseconds; change the logic to treat the server as ready immediately when
the check is disabled (i.e., if need_check_namesrv_ready.load(Acquire) == false
then namesrv_ready = true), use Acquire for the atomic load to pair with any
Release store, and compute wait_millis safely (e.g., use checked_mul or
saturating_mul on wait_seconds as u64 to produce milliseconds) before comparing
elapsed_millis to avoid overflow.
3 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4020
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Bug Fixes
Refactor