[ISSUE #5335]✨Improve pull_request_hold_service accessor to return Option<&T> in BrokerRuntimeInner

[WaterWhisperer]

Which Issue(s) This PR Fixes(Closes)

Fixes #5335

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

• Refactor
  • Improved internal API consistency for accessing pull request services.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[rocketmq-rust-bot]

🔊@WaterWhisperer 🚀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]

Walkthrough

This PR improves the API design of the pull_request_hold_service() accessor method in BrokerRuntimeInner by changing its return type from &Option<T> to Option<&T>, using the .as_ref() method for conversion. This provides a more idiomatic Rust interface.

Changes

Cohort / File(s)	Summary
API signature and implementation update rocketmq-broker/src/broker_runtime.rs	Return type changed from &Option<PullRequestHoldService<MS>> to Option<&PullRequestHoldService<MS>>; implementation updated to use .as_ref() for proper Option unwrapping

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

• #5434: Applies the same &Option<T> → Option<&T> accessor pattern to multiple fields in broker_runtime.rs
• #5368: Modifies the same pull_request_hold_service field with complementary immutable/mutable accessor conversions
• #5398: Uses identical accessor refactoring pattern in the same broker_runtime.rs file

Suggested labels

enhancement✨, Difficulty level/Easy, approved, auto merge

Suggested reviewers

• SpaceXCN
• mxsm
• TeslaRustor

Poem

🐰 A reference wrapped in options turned inside out,
No more boxes within the &, no longer devout!
Now callers can pattern match with grace,
As Option-wrapped references find their place. ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: improving the pull_request_hold_service accessor return type from &Option to Option<&T>, which aligns with the code changes.
Linked Issues check	✅ Passed	The PR successfully implements the enhancement requested in issue #5335: the method signature was changed from returning &Option<PullRequestHoldService> to Option<&PullRequestHoldService>.
Out of Scope Changes check	✅ Passed	All changes are within scope—only the pull_request_hold_service method signature and implementation were modified, directly addressing issue #5335 requirements.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

Fix all issues with AI Agents 🤖

In @rocketmq-broker/src/broker_runtime.rs:
- Around line 1812-1814: The new pull_request_hold_service method now returns
Option<&PullRequestHoldService<MS>>, so remove redundant
.as_ref()/.as_ref().unwrap() call chains at the listed call sites and use direct
pattern matching or Option handling instead; update
notify_message_arriving_listener.rs (lines ~54-56), pull_request_hold_service.rs
(around lines ~76 and ~82), and default_pull_message_result_handler.rs (around
lines ~248-250) to mirror notify_min_broker_id_handler.rs by using if let
Some(svc) = self.broker.pull_request_hold_service() (or match) and then use svc
directly, avoiding unwraps and preserving borrow semantics.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08613f5 and 8100897.

📒 Files selected for processing (1)

• rocketmq-broker/src/broker_runtime.rs

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-05-10T06:20:00.401Z

Learnt from: 578223592
Repo: mxsm/rocketmq-rust PR: 3240
File: rocketmq-namesrv/src/kvconfig/kvconfig_mananger.rs:69-71
Timestamp: 2025-05-10T06:20:00.401Z
Learning: In RocketMQ Rust, while dashmap is concurrent-safe, returning a direct reference to a DashMap can break encapsulation by allowing external code to modify the map without triggering persistence methods like `persist()`. Consider returning a read-only view, iterator methods, or a snapshot instead.

Applied to files:

• rocketmq-broker/src/broker_runtime.rs

⏰ 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). (2)

• GitHub Check: Check (fmt + clippy)
• GitHub Check: auto-approve

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.19%. Comparing base (08613f5) to head (8100897).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
rocketmq-broker/src/broker_runtime.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5459   +/-   ##
=======================================
  Coverage   38.19%   38.19%           
=======================================
  Files         813      813           
  Lines      110210   110210           
=======================================
  Hits        42097    42097           
  Misses      68113    68113           

☔ 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.

[rocketmq-rust-bot]

LGTM - All CI checks passed ✅