[ISSUE #10797] Propagate proxy end transaction result - #10798
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.
Propagates the async result of proxy end-transaction operations to the remoting response, ensuring success is only returned after the underlying endTransaction(...) future completes (and failures map to the appropriate error response).
Changes:
- Make
TransactionActivitywrite the response asynchronously aftermessagingProcessor.endTransaction(...)completes. - Route async failures through the existing remoting error response mapping.
- Add regression tests for success and broker-exception failure responses.
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/TransactionActivity.java | Defers success response until the end-transaction future completes; maps async failures to error responses. |
| proxy/src/test/java/org/apache/rocketmq/proxy/remoting/activity/TransactionActivityTest.java | Adds unit coverage for async success and async failure response behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| public void testEndTransactionWritesSuccessAfterFutureCompletes() throws Exception { | ||
| when(messagingProcessor.endTransaction(any(), eq("topic"), eq("transactionId"), eq("msgId"), | ||
| eq("producerGroup"), eq(TransactionStatus.COMMIT), eq(false))) | ||
| .thenReturn(CompletableFuture.completedFuture(null)); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10798 +/- ##
=============================================
- Coverage 48.60% 48.51% -0.10%
+ Complexity 13690 13653 -37
=============================================
Files 1381 1381
Lines 101464 101469 +5
Branches 13187 13187
=============================================
- Hits 49318 49228 -90
- Misses 46158 46222 +64
- Partials 5988 6019 +31 ☔ 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.
Summary
Propagates the end-transaction result from the proxy to the caller instead of silently discarding it. Ensures transaction status is correctly communicated. Clean change with proper error handling.
LGTM.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
LGTM — defensive improvement with proper error handling and test coverage.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix with proper validation and test coverage. LGTM.
Automated review by github-manager-bot
|
Addressed the review comments. The asynchronous failure path now unwraps CompletionException before the existing remoting error mapper runs, so MQBrokerException response codes and remarks are retained. The existing test suite already returns a completed ChannelFuture and verifies that no success response is written before the pending endTransaction future completes. Verified with: mvn -q -pl proxy -am -Dtest=TransactionActivityTest -Dsurefire.failIfNoSpecifiedTests=false test |
70960ba to
e10c126
Compare
Which Issue(s) This PR Fixes
Fixes #10797
Brief Description
messagingProcessor.endTransaction(...)completesHow Did You Test This Change?
mvn -pl proxy -Dtest=TransactionActivityTest testThe target test passed: 2 tests, 0 failures, 0 errors. The Maven run completed with BUILD SUCCESS; the output still includes existing Jacoco/JDK17 instrumentation warnings.