[ISSUE #4031]🚀Add reject_request method to handle slave broker read conditions in PullMessageProcessor

[mxsm]

Which Issue(s) This PR Fixes(Closes)

Fixes #4031

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

• Bug Fixes
  • Pull requests to slave brokers are now immediately rejected when slave reads are disabled, returning a clear “SlaveNotAvailable” response with an explanatory message. This prevents ambiguous behavior and avoids unnecessary processing, improving reliability and response clarity for clients.

[coderabbitai]

Walkthrough

Adds a reject_request method to PullMessageProcessor to pre-check and reject pull requests when the broker is Slave and slave_read_enable is false, returning a SlaveNotAvailable response. Includes necessary imports and integrates the method into the RequestProcessor impl.

Changes

Cohort / File(s)	Summary of Changes
Pull request pre-check in PullMessageProcessor rocketmq-broker/src/processor/pull_message_processor.rs	Added reject_request method in RequestProcessor impl to reject reads on Slave when slave_read_enable is false; imported BrokerRole and RejectRequestResponse; established early-return path with SlaveNotAvailable remark.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant C as Client
    participant P as PullMessageProcessor
    participant B as Broker Config/Role

    C->>P: PullMessage Request
    rect rgba(220,240,255,0.5)
    note right of P: New pre-check
    P->>B: Check role and slave_read_enable
    alt Broker is Slave AND slave_read_enable=false
        P-->>C: RemotingResponse: SlaveNotAvailable (reject)
    else Proceed
        P->>P: Continue normal pull handling
        P-->>C: Pull result/other response
    end
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (4 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title accurately and directly describes the primary change—adding a reject_request method to PullMessageProcessor to handle slave-read conditions—and it references the linked issue, so it is related to the changeset; its wording is specific and meaningful to reviewers. The title contains an emoji and an "ISSUE #4031" prefix which are stylistic noise but do not make the title incorrect.
Linked Issues Check	✅ Passed	The changes implement a reject_request method on PullMessageProcessor that checks slave_read_enable and broker role and returns a RejectRequestResponse with a SlaveNotAvailable remoting response, which directly satisfies the linked issue's coding objective to add such a method to handle slave broker read conditions. The PR body lacks test or usage details, but per the linked-issue requirements the code-level change fulfills the stated objective.
Out of Scope Changes Check	✅ Passed	Based on the provided summary, only rocketmq-broker/src/processor/pull_message_processor.rs was modified to add the reject_request method and necessary imports, and there are no other files or unrelated features changed, so I find no out-of-scope changes. The change appears focused on the linked objective and does not introduce unrelated functionality.

Poem

I twitch my nose at the broker’s gate,
“Slave not ready? Then I’ll wait.”
A gentle hop, a prudent check,
No carrots pulled from a quiet deck.
When flags allow and roles align,
I’ll nibble messages, fresh and fine. 🥕✨

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature-4031

---

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

[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💥.

[Copilot]

Pull Request Overview

This PR adds a reject_request method to the PullMessageProcessor to handle read conditions for slave brokers, specifically rejecting read requests when slave reading is disabled on a slave broker.

• Adds imports for BrokerRole and RejectRequestResponse types
• Implements reject_request method that returns a rejection response when slave read is disabled on slave brokers

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

Grammar error in the error message. Should be 'the slave broker does not allow reading' or 'the slave broker is not allowed to read'.

Suggested change

	"the slave broker not allow to read",
	"the slave broker does not allow reading",

[codecov]

Codecov Report

❌ Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.52%. Comparing base (d5c9911) to head (a3354bf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...tmq-broker/src/processor/pull_message_processor.rs	0.00%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4032      +/-   ##
==========================================
- Coverage   26.52%   26.52%   -0.01%     
==========================================
  Files         565      565              
  Lines       80766    80779      +13     
==========================================
  Hits        21426    21426              
- Misses      59340    59353      +13     

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

rocketmq-broker/src/processor/pull_message_processor.rs (2)

115-128: Early reject on slave read disabled is correct; refine the remark for clarity.

Logic aligns with the PR objective. Improve the user-facing remark (grammar + context) and include broker identity for diagnostics.

Apply:

 fn reject_request(&self, _code: i32) -> RejectRequestResponse {
     if !self.broker_runtime_inner.broker_config().slave_read_enable
         && self.broker_runtime_inner.message_store_config().broker_role == BrokerRole::Slave
     {
         return (
             true,
             Some(RemotingCommand::create_response_command_with_code_remark(
                 ResponseCode::SlaveNotAvailable,
-                "the slave broker not allow to read",
+                format!(
+                    "The broker[{}] is in slave mode; reads are disabled (slaveReadEnable=false).",
+                    self.broker_runtime_inner.broker_config().broker_ip1
+                ),
             )),
         );
     }
     (false, None)
 }

I can also add a focused unit test to cover both branches (reject/allow). Want me to draft it?

---

103-111: Nit: incorrect processor name in unsupported-code remark.

Message says “ClientManageProcessor” in PullMessageProcessor. Update for accuracy.

-                        "ClientManageProcessor request code {} not supported",
+                        "PullMessageProcessor request code {} not supported",

📜 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 d5c9911 and a3354bf.

📒 Files selected for processing (1)

• rocketmq-broker/src/processor/pull_message_processor.rs (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

rocketmq-broker/src/processor/pull_message_processor.rs (4)

rocketmq-broker/src/processor/send_message_processor.rs (1)

• reject_request (133-167)

rocketmq-broker/src/processor.rs (2)

• reject_request (142-160)
• reject_request (235-251)

rocketmq-remoting/src/runtime/processor.rs (1)

• reject_request (35-37)

rocketmq-remoting/src/protocol/remoting_command.rs (1)

• create_response_command_with_code_remark (196-204)

⏰ 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 (macos-latest, nightly)
• GitHub Check: build (windows-latest, nightly)
• GitHub Check: build (ubuntu-latest, nightly)
• GitHub Check: build (macos-latest, stable)
• GitHub Check: build (windows-latest, stable)
• GitHub Check: build (ubuntu-latest, stable)
• GitHub Check: test
• GitHub Check: build
• GitHub Check: auto-approve

🔇 Additional comments (2)

rocketmq-broker/src/processor/pull_message_processor.rs (2)

21-21: Import of BrokerRole is appropriate.

Required for the role check in reject_request; no issues.

---

46-46: Import of RejectRequestResponse is correct.

Matches the trait’s expected return type; no issues.

[rocketmq-rust-bot]

LGTM