iloveeyjafjalla opened a new pull request, #11374:
URL: https://github.com/apache/rocketmq/pull/11374
### Which Issue(s) This PR Fixes
- Fixes #11373
### Brief Description
`RemotingProtocolServer` constructed `PopMessageActivity`,
`AckMessageActivity` and `ChangeInvisibleTimeActivity` but then
registered other processors for their request codes:
```java
remotingServer.registerProcessor(RequestCode.POP_MESSAGE,
pullMessageActivity, this.pullMessageExecutor);
remotingServer.registerProcessor(RequestCode.ACK_MESSAGE,
consumerManagerActivity, this.updateOffsetExecutor);
remotingServer.registerProcessor(RequestCode.CHANGE_MESSAGE_INVISIBLETIME,
consumerManagerActivity, this.updateOffsetExecutor);
```
Neither processor can serve those codes:
- `PullMessageActivity` decodes a `PullMessageRequestHeader` and unboxes
`getSysFlag()`, while a POP request carries a
`PopMessageRequestHeader`, so a POP never reaches the message path;
- `ConsumerManagerActivity#processRequest0` has no case for the two
acknowledgment codes and returns `null`, and
`AbstractRemotingActivity#processRequest` only writes a response when it
is non-null — so the caller receives **no
response at all**: it fails by its own timeout and the ack is never
applied (the message becomes visible again and is
redelivered).
The change registers the three existing activities for their codes. The
executors are unchanged (`pullMessageExecutor` for
POP, `updateOffsetExecutor` for the acknowledgments), so this only fixes
which processor receives the request.
### How Did You Test This Change?
New test
`RemotingProtocolServerTest#shouldRegisterThePopAckAndChangeInvisibleActivities`:
it builds a real
`RemotingProtocolServer`, registers the processors onto a mocked
`RemotingServer`, and asserts that `POP_MESSAGE` is served by
`PopMessageActivity`, `ACK_MESSAGE` by `AckMessageActivity` and
`CHANGE_MESSAGE_INVISIBLETIME` by
`ChangeInvisibleTimeActivity` (captured via `ArgumentCaptor`).
Results on Temurin JDK 8 (same JDK as CI), base `9ea2ccdc2` plus this patch:
- before the fix: `AssertionError: POP_MESSAGE has to be served by the pop
activity`
- after the fix: `RemotingProtocolServerTest`, `PullMessageActivityTest`,
`AbstractRemotingActivityTest`,
`SendMessageActivityTest`, `RecallMessageActivityTest`,
`GetTopicRouteActivityTest` — 31 tests, 0 failures
- `mvn -B -ntp -pl proxy package` with checkstyle + spotbugs enabled: `BUILD
SUCCESS`
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
