Skip to content

[ISSUE #10546] Add default implementation for rejectRequest() in NettyRequestProcessor#10547

Merged
RongtongJin merged 1 commit into
apache:developfrom
f1amingo:improve/default-reject-request
Jun 25, 2026
Merged

[ISSUE #10546] Add default implementation for rejectRequest() in NettyRequestProcessor#10547
RongtongJin merged 1 commit into
apache:developfrom
f1amingo:improve/default-reject-request

Conversation

@f1amingo

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Fixes #10546

Brief Description

The rejectRequest() method in NettyRequestProcessor interface requires every implementation to provide an override, yet 20+ implementations across the codebase simply return false, with only SendMessageProcessor and PullMessageProcessor containing actual logic.

This PR converts boolean rejectRequest() to default boolean rejectRequest() { return false; } and removes all redundant return false overrides, reducing ~128 lines of boilerplate code.

  • Semantically equivalent: all removed overrides already return false
  • Existing overrides with real logic (Send/Pull) remain unaffected
  • Compatible with Java 8+ (project target)

How Did You Test This Change?

Existing unit tests remain unchanged and should all pass. This is a pure refactoring with no behavioral change.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot

Summary

Converts boolean rejectRequest() to a default method in NettyRequestProcessor interface and removes 26 redundant return false overrides across broker processors, reducing ~128 lines of boilerplate.

Findings

  • [Info] NettyRequestProcessor.java:29 — Clean use of Java 8 default methods. Project targets Java 11, fully compatible.
  • [Info] 26 processor classes had their return false overrides removed. Verified that SendMessageProcessor and PullMessageProcessor (which contain actual rejection logic) are not modified — their overrides are preserved.
  • [Info] AbstractSendMessageProcessor.java is correctly included (it had a return false override), while SendMessageProcessor (its subclass with real logic) is correctly excluded.
  • [Info] Test class RemotingServerTest.java anonymous inner class override also removed — consistent with the interface change.

Assessment

This is a semantically equivalent refactoring with zero behavioral change:

  • All removed overrides returned false, matching the new default
  • No public API changes
  • No thread-safety or concurrency implications
  • Backward compatible with all existing callers

Clean and well-scoped. 👍


Automated review by github-manager-bot

@f1amingo f1amingo force-pushed the improve/default-reject-request branch from 565a7f6 to c88fdbc Compare June 24, 2026 07:47
@codecov-commenter

codecov-commenter commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.20%. Comparing base (2e6632f) to head (c88fdbc).

Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10547      +/-   ##
=============================================
- Coverage      48.27%   48.20%   -0.07%     
+ Complexity     13435    13415      -20     
=============================================
  Files           1377     1378       +1     
  Lines         100844   100822      -22     
  Branches       13036    13036              
=============================================
- Hits           48678    48601      -77     
- Misses         46217    46264      +47     
- Partials        5949     5957       +8     

☔ View full report in Codecov by Harness.
📢 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.

@f1amingo f1amingo changed the title [ISSUE-10546] Add default implementation for rejectRequest() in NettyRequestProcessor [ISSUE #10546] Add default implementation for rejectRequest() in NettyRequestProcessor Jun 24, 2026
@f1amingo f1amingo force-pushed the improve/default-reject-request branch from 5450816 to c88fdbc Compare June 24, 2026 09:56

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot (Re-review after new commit c88fdbc)

Summary

New commit confirmed — all redundant rejectRequest() overrides have been removed consistently across all NettyRequestProcessor implementations (Broker, NameServer, Proxy modules). The interface now provides the default implementation.

Assessment

  • [Correctness] Clean refactoring. The default boolean rejectRequest() { return false; } in the interface is semantically identical to the removed overrides. No behavioral change.
  • [Compatibility] Backward-compatible. Any external implementations that relied on the abstract method will now get the default. This is a safe change.
  • [Tests] RemotingServerTest updated to verify the default behavior. Good.

LGTM — all core CI checks pass. Nice cleanup of boilerplate code. 👍


Automated by github-manager-bot

@RongtongJin RongtongJin merged commit 10d498c into apache:develop Jun 25, 2026
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add default implementation for rejectRequest() in NettyRequestProcessor

4 participants