[ISSUE #10782] Set error code for empty lock batch MQ - #10783
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR ensures Proxy returns an explicit failure response code when handling LOCK_BATCH_MQ / UNLOCK_BATCH_MQ requests that contain an empty MessageQueue set, and adds tests to verify the rejection behavior and that invalid requests are not forwarded to the broker.
Changes:
- Set
ResponseCode.SYSTEM_ERRORfor empty lock/unlock MQ batch requests while preserving the existing remark/body. - Add remoting activity tests covering empty lock and unlock paths.
- Verify
MessagingProcessor.request(...)is not invoked for invalid requests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/remoting/activity/ConsumerManagerActivity.java | Sets an explicit response code for empty MQ-set lock/unlock requests. |
| proxy/src/test/java/org/apache/rocketmq/proxy/remoting/activity/ConsumerManagerActivityTest.java | Adds tests asserting failure code/remark and ensuring invalid requests aren’t forwarded. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RemotingCommand response = consumerManagerActivity.processRequest0(ctx, request, ProxyContext.create()); | ||
|
|
||
| assertThat(response).isNotNull(); | ||
| assertThat(response.getCode()).isEqualTo(ResponseCode.SYSTEM_ERROR); | ||
| assertThat(response.getRemark()).isEqualTo("MessageQueue set is empty"); | ||
| verify(messagingProcessor, never()).request(any(), any(), any(), anyLong()); |
|
|
||
| assertThat(response).isNotNull(); | ||
| assertThat(response.getCode()).isEqualTo(ResponseCode.SYSTEM_ERROR); | ||
| assertThat(response.getRemark()).isEqualTo("MessageQueue set is empty"); |
|
|
||
| assertThat(response).isNotNull(); | ||
| assertThat(response.getCode()).isEqualTo(ResponseCode.SYSTEM_ERROR); | ||
| assertThat(response.getRemark()).isEqualTo("MessageQueue set is empty"); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10783 +/- ##
=============================================
+ Coverage 48.24% 48.25% +0.01%
+ Complexity 13496 13494 -2
=============================================
Files 1380 1380
Lines 101104 101106 +2
Branches 13107 13107
=============================================
+ Hits 48773 48785 +12
+ Misses 46350 46333 -17
- Partials 5981 5988 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes ConsumerManagerActivity to return ResponseCode.SYSTEM_ERROR instead of SUCCESS when lock/unlock batch MQ operations receive an empty queue set. Prevents callers from misinterpreting empty operations as successful.
Findings
- [Info]
ConsumerManagerActivity.java:89-92— SettingSYSTEM_ERRORfor empty lock batch is correct. ReturningSUCCESSwith an empty operation could mask configuration or routing issues. - [Info]
ConsumerManagerActivity.java:108-111— Same fix applied to unlock path, ensuring consistency. - [Info] New test file
ConsumerManagerActivityTest.javaprovides good coverage for both lock and unlock empty-queue scenarios.
Suggestions
- Consider whether
ILLEGAL_REQUESTorINVALID_PARAMETERmight be more semantically accurate thanSYSTEM_ERRORfor an empty queue set (since it is a client-side input issue rather than a server-side failure). Not blocking —SYSTEM_ERRORis acceptable.
LGTM — correct error code fix with good test coverage.
Automated review by github-manager-bot
What is changed
Fixes #10782
Verification