[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.
| ).thenRun(() -> writeResponse(ctx, context, request, response)) | ||
| .exceptionally(t -> { | ||
| writeErrResponse(ctx, context, request, t); | ||
| return null; | ||
| }); |
| private ChannelHandlerContext ctx = new SimpleChannelHandlerContext(new SimpleChannel(null, "0.0.0.0:0", "1.1.1.1:1")) { | ||
| @Override | ||
| public ChannelFuture writeAndFlush(Object msg, ChannelPromise promise) { | ||
| return null; | ||
| } | ||
| }; |
| @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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10798 +/- ##
=============================================
- Coverage 48.34% 48.25% -0.10%
+ Complexity 13527 13498 -29
=============================================
Files 1380 1380
Lines 101104 101142 +38
Branches 13107 13120 +13
=============================================
- Hits 48882 48807 -75
- Misses 46267 46342 +75
- Partials 5955 5993 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.