Conversation
use TimeUtils::current_millis in stats_item
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
|
🔊@pranshu314 🚀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
WalkthroughRefactored Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-common/src/common/stats/stats_item.rs (1)
86-93:⚠️ Potential issue | 🟡 MinorDocumentation is inconsistent with new initialization behavior.
The doc comment states "A value of
0means the item has never been written to," butlast_update_timestampis now initialized tocurrent_millis()(line 47), so it will never be0for a newly constructed item. The documentation should be updated to reflect the new semantics.📝 Proposed doc update
/// Returns the wall-clock timestamp (milliseconds since UNIX epoch) of the last - /// [`add`](Self::add) or [`increment`](Self::increment) call. - /// - /// A value of `0` means the item has never been written to. + /// [`add`](Self::add) or [`increment`](Self::increment) call, or the creation + /// time if the item has never been written to. #[inline] pub fn get_last_update_timestamp(&self) -> u64 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-common/src/common/stats/stats_item.rs` around lines 86 - 93, The doc comment for get_last_update_timestamp is out of date because last_update_timestamp is initialized to current_millis() (so a newly constructed StatsItem will not return 0); update the documentation on get_last_update_timestamp (and any related doc on initialization) to state that it returns the wall-clock timestamp of the last add/increment or the creation time (initialized to current_millis()), and remove or change the sentence claiming "A value of `0` means the item has never been written to" to reflect the new semantics of last_update_timestamp.
🧹 Nitpick comments (1)
rocketmq-common/src/common/stats/stats_item.rs (1)
156-159: Consider cachingcurrent_millis()to ensure consistent timestamps.When the list is empty,
current_millis()is called twice (lines 157 and 159), which can introduce a minor timing drift between the backdated and current snapshots. Storing the value once would ensure the time difference is exactly the intended offset.♻️ Proposed refactor
pub fn sampling_in_seconds_internal( cs_list: &Mutex<LinkedList<CallSnapshot>>, current_value: u64, current_times: u64, ) { let mut cs_list = cs_list.lock(); + let now = current_millis(); if cs_list.is_empty() { - cs_list.push_back(CallSnapshot::new(current_millis() - 10 * 1000, 0, 0)); + cs_list.push_back(CallSnapshot::new(now - 10 * 1000, 0, 0)); } - cs_list.push_back(CallSnapshot::new(current_millis(), current_times, current_value)); + cs_list.push_back(CallSnapshot::new(now, current_times, current_value)); if cs_list.len() > 7 { cs_list.pop_front(); } }The same pattern applies to
sampling_in_minutes(lines 168-170) andsampling_in_hour(lines 179-181).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-common/src/common/stats/stats_item.rs` around lines 156 - 159, Cache the timestamp once instead of calling current_millis() repeatedly so the backdated and current CallSnapshot use the exact intended offsets: capture let now = current_millis() and use now and now - 10 * 1000 when creating CallSnapshot entries for cs_list in the block shown, and apply the same change where sampling_in_minutes and sampling_in_hour build their snapshots; update usages of CallSnapshot::new(...) to use the cached now variable to ensure consistent timestamps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rocketmq-common/src/common/stats/stats_item.rs`:
- Around line 19-21: Remove the now-unused import "use std::time::SystemTime;"
from the top of stats_item.rs (the code now uses current_millis via use
crate::TimeUtils::current_millis;), search for any remaining references to
SystemTime in the same file and delete them if none exist, then run cargo
check/cargo build to ensure no other usages remain.
---
Outside diff comments:
In `@rocketmq-common/src/common/stats/stats_item.rs`:
- Around line 86-93: The doc comment for get_last_update_timestamp is out of
date because last_update_timestamp is initialized to current_millis() (so a
newly constructed StatsItem will not return 0); update the documentation on
get_last_update_timestamp (and any related doc on initialization) to state that
it returns the wall-clock timestamp of the last add/increment or the creation
time (initialized to current_millis()), and remove or change the sentence
claiming "A value of `0` means the item has never been written to" to reflect
the new semantics of last_update_timestamp.
---
Nitpick comments:
In `@rocketmq-common/src/common/stats/stats_item.rs`:
- Around line 156-159: Cache the timestamp once instead of calling
current_millis() repeatedly so the backdated and current CallSnapshot use the
exact intended offsets: capture let now = current_millis() and use now and now -
10 * 1000 when creating CallSnapshot entries for cs_list in the block shown, and
apply the same change where sampling_in_minutes and sampling_in_hour build their
snapshots; update usages of CallSnapshot::new(...) to use the cached now
variable to ensure consistent timestamps.
| use std::time::SystemTime; | ||
|
|
||
| use crate::TimeUtils::current_millis; |
There was a problem hiding this comment.
Remove unused SystemTime import.
The std::time::SystemTime import on line 19 is no longer used after refactoring to current_millis(). This dead import should be removed.
🧹 Proposed fix
use std::fmt;
use std::sync::atomic::AtomicU64;
use std::sync::atomic::Ordering;
-use std::time::SystemTime;
use crate::TimeUtils::current_millis;📝 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.
| use std::time::SystemTime; | |
| use crate::TimeUtils::current_millis; | |
| use std::fmt; | |
| use std::sync::atomic::AtomicU64; | |
| use std::sync::atomic::Ordering; | |
| use crate::TimeUtils::current_millis; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-common/src/common/stats/stats_item.rs` around lines 19 - 21, Remove
the now-unused import "use std::time::SystemTime;" from the top of stats_item.rs
(the code now uses current_millis via use crate::TimeUtils::current_millis;),
search for any remaining references to SystemTime in the same file and delete
them if none exist, then run cargo check/cargo build to ensure no other usages
remain.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6576 +/- ##
==========================================
- Coverage 41.96% 41.92% -0.04%
==========================================
Files 954 955 +1
Lines 133233 133300 +67
==========================================
- Hits 55906 55887 -19
- Misses 77327 77413 +86 ☔ 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
4 participants
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit