Repository navigation
[ISSUE #11356] fix(proxy): hash admin message groups with the data plane's queue hash - #11357
Open
iloveeyjafjalla wants to merge 1 commit into
Open
iloveeyjafjalla wants to merge 1 commit into
iloveeyjafjalla wants to merge 1 commit into
Conversation
…ata plane's queue hash AdminSendMessage resolved the write queue of a message group with Math.floorMod, while the gRPC data plane resolves it with Guava's consistent hash (SendMessageActivity#select). The two functions disagree for the same group and queue count, so a message injected through the admin surface could land on a queue the group's producers never write to and split the group across queues on the quiet, breaking its FIFO order with an OK status. Use the same hash the data plane uses and cover the parity with a unit test on the QueueSelector the RPC hands to MessagingProcessor#sendMessage.
RockteMQ-AI
approved these changes
Oct 12, 2026
RockteMQ-AI
left a comment
Contributor
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fix admin path queue hash mismatch: use Hashing.consistentHash instead of Math.floorMod so admin-injected messages land on the same queue as the data plane, preserving FIFO ordering.
Findings
- [Info]
ProxyAdminGrpcService.java:1436— The 1-line fix correctly aligns the admin path with the data plane's hash function (SendMessageActivity.java:414). The no-message-group branch (random selection) is unchanged, which is correct. - [Info]
ProxyAdminGrpcServiceTest.java— The new test is well-designed: it captures theQueueSelector, runs it against mocked queues, and asserts the selected queue matches the data plane's formula. The assertion that the two formulas differ for the test key (assertNotEquals) prevents silent regression if the old mapping comes back.
Suggestions
No blocking concerns. This is a minimal, targeted fix with clear problem description and solid test coverage.
Automated review by github-manager-bot
This was referenced Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Issue(s) This PR Fixes
Brief Description
AdminSendMessageresolved the write queue of a message group withMath.floorMod(shardingKey.hashCode(), queues.size()), while the gRPC data plane resolves it with Guava'sconsistent hash (
SendMessageActivity.java:414). The two functions disagree for the same group and queue count —for message group
groupA-fifoover 3 writable queues,floorModpicks queue 2 andconsistentHashpicksqueue 0 — so a message injected through the admin surface could land on a queue that the group's producers never
write to. Per-group FIFO order only holds within a single queue, so the group's messages could be consumed out
of order, and the RPC still answered
OK, so nothing told the operator the group was split.selectQueue's javadoc states the intent ("A FIFO message must keep going to the queue its message group hashesto") and
docs/proxy-admin.md§D4 documents this path as "system properties honoured for timer / FIFO messages",so the mapping is meant to match the data plane.
The change makes the admin path use the same hash function the data plane uses. Nothing else changes: the
no-message-group branch keeps spreading over the writable queues.
How Did You Test This Change?
New unit test
ProxyAdminGrpcServiceTest#adminSendMessageRoutesMessageGroupToTheDataPlaneQueueTest: it capturesthe
QueueSelectorthe RPC hands toMessagingProcessor#sendMessage, runs it against a mockedMessageQueueViewwith 3 writable queues, and asserts the selected queue is the one the data plane's formula picks for that group.
It also asserts the two formulas differ for the test key, so the test cannot silently pass if the old mapping
comes back.
Results on Temurin JDK 8 (same JDK as CI), base
9ea2ccdc2plus this patch:expected:<...queueId=0...> but was:<...queueId=2...>Tests run: 1, Failures: 0, Errors: 0, Skipped: 0Tests run: 61, Failures: 0, Errors: 0, Skipped: 0(mvn -B -ntp -pl proxy -am test -Dtest=ProxyAdminGrpcServiceTest)This change was prepared with AI assistance (analysis, patch and test drafting); the diff and every result above
were reproduced locally by running the module's tests against the base commit and the patched tree.