Ethan-Xingyue commented on PR #1166: URL: https://github.com/apache/incubator-seata-go/pull/1166#issuecomment-5528977028
LGTM overall — the fix is correct (`TYPE_GLOBAL_COMMIT = 7` matches `GlobalCommitRequest.getTypeCode()` on the Java side), and I confirmed the new test fails without the fix and passes with it. A few comments: 1. **Same bug in RM remoting.** `pkg/rm/remoting/grpc/rm_remoting.go:65` sets `TYPE_BRANCH_COMMIT` on `BranchReportRequestProto`; it should be `TYPE_BRANCH_STATUS_REPORT` (Java `BranchReportRequest.getTypeCode()` returns 13). Could you fix it in this PR with the same kind of assertion, or open a follow-up? `RegisterTMRequestProto` (`channel_manager.go:299`) and the two branch responses in `pkg/remoting/processor/client/` don't set `AbstractMessage` at all — worth tracking in the same follow-up. 2. **PR description / release note.** The TC does not read the inner `messageType`: `GrpcDecoder` only uses the outer `GrpcMessageProto.messageType` for heartbeat detection, `GrpcSerializer` routes by the `Any` typeUrl, and `GlobalCommitRequestConvertor.convert2Model` ignores the field. So this is a wire-format consistency fix with the Java client rather than a routing bug. Suggest rewording to avoid implying user-visible impact. Also, the bug was introduced in #1113 after v2.1.0, so it has not shipped in any release. 3. **nit:** the assertions inside the mock are skipped if `Commit()` never calls `SendSyncRequest`, so the test could pass vacuously. A `called` flag asserted after `Commit()` would close that gap. Alternatively, the two assertions could live in the success case of the existing table-driven `TestGrpcGlobalTransactionCommit`. 4. Per `CONTRIBUTING.md`, please add a line to `changes/dev.md`. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
