[ISSUE #10778] Skip empty priority queue groups - #10779
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a robustness bug in Proxy route queue selection by making MessageQueuePenalizer.selectLeastPenaltyWithPriority() tolerate empty priority buckets and match the existing “empty input => null” contract used by selectLeastPenalty().
Changes:
- Skip empty (and null) priority groups when scanning for the least-penalty queue.
- Return
nullwhen all priority groups are empty (instead of returning a(null, Integer.MAX_VALUE)pair / throwing). - Add regression tests covering all-empty and partially-empty priority group inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/service/route/MessageQueuePenalizer.java | Skips empty priority buckets and returns null when no queue can be selected. |
| proxy/src/test/java/org/apache/rocketmq/proxy/service/route/MessageQueuePenalizerTest.java | Adds tests to prevent regressions for empty priority-group handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10779 +/- ##
==========================================
Coverage 48.24% 48.24%
- Complexity 13496 13497 +1
==========================================
Files 1380 1380
Lines 101104 101108 +4
Branches 13107 13109 +2
==========================================
+ Hits 48773 48779 +6
+ Misses 46350 46347 -3
- Partials 5981 5982 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Adds null/empty guard for the selected queue in DefaultConsumerGroupAssignor.assign(), preventing NullPointerException when selectLeastPenalty() returns null or when all priority groups are empty.
Findings
- [Info] The fix correctly handles the case where all queue groups are empty or
selectLeastPenalty()returns null. The null check onbestQueueat the end prevents NPE when no valid queue is found. - [Info] The removal of double spaces (
if (queueAndPenalty == null)) is a good minor cleanup. - [Info] Test coverage is solid —
testAssignAllEmptyPriorityGroupsverifies the FORBIDDEN result, andtestAssignPartialEmptyPriorityGroupsverifies partial-empty scenarios still work.
Suggestions
- Minor: the
if (bestQueue == null)check at the end could include a brief inline comment explaining that this handles the "all groups empty" case for future readers.
LGTM.
Automated review by github-manager-bot
What changed
nullwhen all priority groups are empty, matching the existingselectLeastPenalty()empty-list contract.Fixes #10778.
Verification
mvn -pl proxy -Dtest=MessageQueuePenalizerTest testResult: BUILD SUCCESS.
MessageQueuePenalizerTestran 23 tests with 0 failures, errors, or skips. Checkstyle and SpotBugs also passed in the Maven run.