[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.34% 48.23% -0.12%
+ Complexity 13527 13494 -33
=============================================
Files 1380 1380
Lines 101104 101142 +38
Branches 13107 13122 +15
=============================================
- Hits 48882 48783 -99
- Misses 46267 46366 +99
- Partials 5955 5993 +38 ☔ 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
RockteMQ-AI
left a comment
There was a problem hiding this comment.
LGTM. Code changes look good.
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.