Skip to content

[ISSUE #6585, #6582, #6579]#6586

Merged
mxsm merged 5 commits intomxsm:mainfrom
oopscompiled:enhancements-4
Mar 1, 2026
Merged

[ISSUE #6585, #6582, #6579]#6586
mxsm merged 5 commits intomxsm:mainfrom
oopscompiled:enhancements-4

Conversation

@oopscompiled
Copy link
Contributor

@oopscompiled oopscompiled commented Feb 28, 2026

Which Issue(s) This PR Fixes(Closes)

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

  • Refactor

    • Renamed message property accessor to align with platform naming conventions (buyer ID accessor updated).
    • Improved broker statistics time tracking by switching to a millisecond-based utility for more accurate scheduling and delay computations.
  • Chores

    • Removed an unnecessary informational diagnostic log from the long-polling hold service to reduce log noise and clutter.

@rocketmq-rust-bot
Copy link
Collaborator

🔊@oopscompiled 🚀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💥.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 28, 2026

Walkthrough

Removed an informational log from long-polling hold service, renamed get_buyer_id to buyer_id on MessageTrait (signature changed), and switched broker stats time retrieval to a centralized millisecond utility.

Changes

Cohort / File(s) Summary
Logging Cleanup
rocketmq-broker/src/long_polling/long_polling_service/pull_request_hold_service.rs
Removed an informational logging block that emitted topic and queue_id; no control flow or data changes.
Trait Method Renaming
rocketmq-common/src/common/message.rs
Renamed trait method get_buyer_id(&self) -> Option<CheetahString>buyer_id(&self) -> Option<CheetahString); implementation unchanged. This is a public API signature change.
Time Utility Consolidation
rocketmq-store/src/stats/broker_stats_manager.rs
Replaced manual SystemTime/UNIX_EPOCH millisecond calculation with current_millis() (imported); internal timing computation updated, no behavioral change to public API.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through logs and nudged a name so neat,
Swapped clocks for a helper, now the minutes meet,
Quiet changes, tidy trail, code crumbs all in sight,
Little paws, soft refactor, everything feels right.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The PR title is a list of issue numbers without descriptive text about the actual changes being made. Replace the title with a descriptive summary of the main changes, such as 'Rename get_buyer_id to buyer_id in MessageTrait and refactor logging' that clearly communicates what was changed.
Linked Issues check ❓ Inconclusive The PR addresses issue #6585 (renaming get_buyer_id to buyer_id) but changes to broker_stats_manager.rs and removal of logging appear unrelated to the single documented issue. Clarify whether issues #6582 and #6579 cover the additional changes (logging removal and stats manager refactoring) or if these are out-of-scope modifications.
Out of Scope Changes check ❓ Inconclusive Only one linked issue details are provided (#6585), but PR includes three unrelated changes: method renaming, logging removal, and time utility refactoring. Provide details for issues #6582 and #6579 to verify whether all changes align with documented requirements, or remove unrelated modifications.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses three small enhancements across the RocketMQ Rust workspace: standardizing time retrieval in broker stats scheduling, clarifying the message buyer-id accessor naming, and removing dead/commented logging code in long polling.

Changes:

  • Switch BrokerStatsManager::compute_initial_delay_to_next_minute to use rocketmq_common::TimeUtils::get_current_millis.
  • Rename MessageTrait::get_buyer_id to MessageTrait::buyer_id.
  • Remove commented-out info! logging from PullRequestHoldService.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
rocketmq-store/src/stats/broker_stats_manager.rs Uses shared TimeUtils helper for current timestamp when aligning scheduled tasks to minute boundaries.
rocketmq-common/src/common/message.rs Renames buyer-id accessor on MessageTrait to buyer_id.
rocketmq-broker/src/long_polling/long_polling_service/pull_request_hold_service.rs Cleans up unused/commented logging in the hold-request scanning loop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -326,7 +326,7 @@ pub trait MessageTrait: Any + Display + Debug {
/// # Returns
///
/// An `Option<String>` containing the buyer ID if it exists, otherwise `None`.
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

The docstring says this returns Option<String>, but the signature returns Option<CheetahString>. Please update the documentation to match the actual return type to avoid misleading API consumers.

Suggested change
/// An `Option<String>` containing the buyer ID if it exists, otherwise `None`.
/// An `Option<CheetahString>` containing the buyer ID if it exists, otherwise `None`.

Copilot uses AI. Check for mistakes.
fn buyer_id(&self) -> Option<CheetahString> {
self.property(&CheetahString::from_static_str(MessageConst::PROPERTY_BUYER_ID))
}

Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

Renaming get_buyer_id to buyer_id is a breaking change for downstream crates implementing/using MessageTrait. Consider keeping a deprecated get_buyer_id default method that forwards to buyer_id() for at least one release to preserve backward compatibility.

Suggested change
/// Deprecated compatibility method for retrieving the buyer ID.
///
/// This method is kept for backward compatibility with older versions
/// of the API. Prefer using [`buyer_id`] instead.
#[deprecated(
note = "get_buyer_id is deprecated, use buyer_id() instead. This method may be removed in a future release."
)]
fn get_buyer_id(&self) -> Option<CheetahString> {
self.buyer_id()
}

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Feb 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 41.84%. Comparing base (7616f22) to head (3688284).
⚠️ Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6586      +/-   ##
==========================================
- Coverage   42.16%   41.84%   -0.32%     
==========================================
  Files         946      956      +10     
  Lines      132145   133557    +1412     
==========================================
+ Hits        55714    55882     +168     
- Misses      76431    77675    +1244     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@rocketmq-rust-bot rocketmq-rust-bot left a comment

Choose a reason for hiding this comment

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

LGTM - All CI checks passed ✅

Copy link
Owner

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

@oopscompiled You can take on more challenging tasks while hoping that one PR solves one problem. Simple PRs can be left for those who have just entered open source.

Copy link
Owner

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@mxsm mxsm merged commit 0ec62c0 into mxsm:main Mar 1, 2026
9 of 14 checks passed
@rocketmq-rust-bot rocketmq-rust-bot added approved PR has approved and removed ready to review waiting-review waiting review this PR labels Mar 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

5 participants