bugfix: in dubbo 3.x version, the consumer can't generate tcc proxy. #6101

laywin · 2023-12-03T04:36:36Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

#6099

Ⅱ. Does this pull request fix one issue?

fixes #6099
fixes #6096

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

# Conflicts: # changes/en-us/2.0.0.md # changes/zh-cn/2.0.0.md # rm-datasource/src/main/java/io/seata/rm/datasource/DataSourceProxy.java # rm-datasource/src/test/java/io/seata/rm/RMHandlerATTest.java

…atic mock 2. throw exception when the db type not support in AT mode 3. add author register info

# Conflicts: # changes/zh-cn/2.x.md

2. remove tryOtherApp when get channel, so it can default use other same resourceId channel but difference app when the is no channel in same app.

� Conflicts: � changes/en-us/2.x.md � changes/zh-cn/2.x.md

codecov · 2023-12-03T16:52:08Z

Codecov Report

Merging #6101 (a6af930) into 2.x (501d431) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #6101      +/-   ##
============================================
- Coverage     48.98%   48.95%   -0.04%     
+ Complexity     4782     4773       -9     
============================================
  Files           913      913              
  Lines         31697    31697              
  Branches       3823     3823              
============================================
- Hits          15528    15518      -10     
- Misses        14632    14644      +12     
+ Partials       1537     1535       -2

Files	Coverage Δ
...n/java/io/seata/core/rpc/netty/ChannelManager.java	`0.00% <ø> (ø)`
...va/io/seata/integration/tx/api/util/DubboUtil.java	`0.00% <0.00%> (ø)`

... and 5 files with indirect coverage changes

funky-eyes · 2023-12-04T01:11:37Z

integration-tx-api/src/main/java/io/seata/integration/tx/api/util/DubboUtil.java

@@ -34,6 +34,8 @@ private DubboUtil() {
    private static final String ALIBABA_DUBBO_PROXY_NAME_PREFIX = "com.alibaba.dubbo.common.bytecode.proxy";
    private static final String APACHE_DUBBO_PROXY_NAME_PREFIX = "org.apache.dubbo.common.bytecode.proxy";

+    private static final String DUBBO_3_X_PARTIAL_PROXY_NAME = "DubboProxy";


Can you only use contains? and not startsWith or endsWith?
只能使用contains？而不是startsWith或者endsWith吗？

there is no fixed prefix or suffix in generated proxy name, i will try later to concat a prefix to judge.
生成代理名字的时候没有固定的前缀和后缀，我看能不能凭借一个前缀来判断.

Deeply analyzing the source code is really excellent.

funky-eyes · 2023-12-04T01:12:26Z

core/src/main/java/io/seata/core/rpc/netty/ChannelManager.java

@@ -399,7 +400,7 @@ public static Channel getChannel(String resourceId, String clientId, boolean try
            }
        }

-        if (resultChannel == null && tryOtherApp) {
+        if (resultChannel == null && BranchType.TCC == branchType) {


为什么要改变入参，而不是在由调用方决定是否开启tryOtherApp
Why change the entry parameter when it's up to the caller to turn on tryOtherApp?

i feel the place convention over configuration is better, method signature is better understood without loss of flexibility.
这个地方感觉用预定大于配置的方法更好一点，不失灵活性的同时方法签名也更好理解.

i feel the place convention over configuration is better, method signature is better understood without loss of flexibility. 这个地方感觉用预定大于配置的方法更好一点，不失灵活性的同时方法签名也更好理解.

I'm not sure if there are any other branch types that need this in the future, but if they all rely on an internal if || it's just too low.
未来不确定有没有其他的分支类型需要这种，如果都靠内部一个if || 这种写法太low了

What about tryOtherApp in AT transaction mode? The original logic is that only AT mode can go here, now only TCC mode can go here?

What about tryOtherApp in AT transaction mode? The original logic is that only AT mode can go here, now only TCC mode can go here?

yeah, because of TCC rpc mode regiter branch in constomer, send order of TC two stage to provider. so it can't find the channel as normal, so tcc model will try other app. @funky-eyes 's opinion there may be a risk in at to find a same resource diff app channel, ex: two instance have status.

是的，因为TCC RPC模式是在消费者端注册分支，TC在二阶段下发指令给服务者端。导致通过消费者端的app去找服务者端的channel的时候会失败，所以tcc 模式会尝试其他app channel. @funky-eyes 的建议在AT模式下发指令到相同资源不同app的端可能存在风险, 比如两台实例可能有状态的情况下.

i feel the place convention over configuration is better, method signature is better understood without loss of flexibility. 这个地方感觉用预定大于配置的方法更好一点，不失灵活性的同时方法签名也更好理解.

I'm not sure if there are any other branch types that need this in the future, but if they all rely on an internal if || it's just too low. 未来不确定有没有其他的分支类型需要这种，如果都靠内部一个if || 这种写法太low了

no problem, i will rollback.

What about tryOtherApp in AT transaction mode? The original logic is that only AT mode can go here, now only TCC mode can go here?

yeah, because of TCC rpc mode regiter branch in constomer, send order of TC two stage to provider. so it can't find the channel as normal, so tcc model will try other app. @funky-eyes 's opinion there may be a risk in at to find a same resource diff app channel, ex: two instance have status.

是的，因为TCC RPC模式是在消费者端注册分支，TC在二阶段下发指令给服务者端。导致通过消费者端的app去找服务者端的channel的时候会失败，所以tcc 模式会尝试其他app channel. @funky-eyes 的建议在AT模式下发指令到相同资源不同app的端可能存在风险, 比如两台实例可能有状态的情况下.

Maybe this is not the scope of this bugfix.

What about tryOtherApp in AT transaction mode? The original logic is that only AT mode can go here, now only TCC mode can go here?

yeah, because of TCC rpc mode regiter branch in constomer, send order of TC two stage to provider. so it can't find the channel as normal, so tcc model will try other app. @funky-eyes 's opinion there may be a risk in at to find a same resource diff app channel, ex: two instance have status.
是的，因为TCC RPC模式是在消费者端注册分支，TC在二阶段下发指令给服务者端。导致通过消费者端的app去找服务者端的channel的时候会失败，所以tcc 模式会尝试其他app channel. @funky-eyes 的建议在AT模式下发指令到相同资源不同app的端可能存在风险, 比如两台实例可能有状态的情况下.

Maybe this is not the scope of this bugfix.

tcc rpc mode can't find the channel, if no this change.

pengten · 2023-12-05T07:56:36Z

server/src/main/java/io/seata/server/coordinator/AbstractCore.java

@@ -192,7 +192,7 @@ protected BranchStatus branchCommitSend(BranchCommitRequest request, GlobalSessi
                                            BranchSession branchSession) throws IOException, TimeoutException {

        BranchCommitResponse response = (BranchCommitResponse) remotingServer.sendSyncRequest(
-            branchSession.getResourceId(), branchSession.getClientId(), request, branchSession.isAT());
+            branchSession.getResourceId(), branchSession.getClientId(), request, branchSession.getBranchType() == BranchType.TCC);


What is the purpose of this change?

slievrly

I cannot agree with this plan. Consider what would happen if there were no multi-tenancy isolation in TCC.

funky-eyes · 2023-12-07T01:10:07Z

I cannot agree with this plan. Consider what would happen if there were no multi-tenancy isolation in TCC.

A long time ago, the version is to allow tcc to be sent to other apps, and in the consumer side of the first stage of registration, the second stage in the provider side of the submission rely on this function, this pr just restore the previous practice
很久以前的版本就是允许tcc下发的其他的app中，而且在consumer侧一阶段注册，二阶段在provider侧提交靠的就是这个功能，这个pr只是还原了以前的做法

funky-eyes

LGTM

slievrly

LGTM

laywin and others added 20 commits November 14, 2023 16:23

undo_log table check optimize

ef782b3

optimize & add modify record

1df81eb

adjust indentation

8e92325

undo_log table check optimize

7482281

optimize & add modify record

522c386

adjust indentation

0444780

add unit test

881aaf4

move pr record

b8f822c

Merge remote-tracking branch 'origin/2.x' into 2.x

fc2e79f

# Conflicts: # changes/en-us/2.0.0.md # changes/zh-cn/2.0.0.md # rm-datasource/src/main/java/io/seata/rm/datasource/DataSourceProxy.java # rm-datasource/src/test/java/io/seata/rm/RMHandlerATTest.java

modify unit test

e36326e

add Github ID

0bf3847

Merge branch '2.x' into 2.x

47d150d

1. update mockito version to 4.5.1 same as seata server to support st…

12f8109

…atic mock 2. throw exception when the db type not support in AT mode 3. add author register info

Merge remote-tracking branch 'origin/2.x' into 2.x

c8a6115

# Conflicts: # changes/zh-cn/2.x.md

import optimize

c5761c7

1. add dubbo 3.x's proxy name to optimize proxy judge.

572192b

2. remove tryOtherApp when get channel, so it can default use other same resourceId channel but difference app when the is no channel in same app.

Merge branch '2.x' of github.com:seata/seata into 2.x

53f3070

� Conflicts: � changes/en-us/2.x.md � changes/zh-cn/2.x.md

add change registry info

2a01c04

add try other app logic in TCC mode when TC send order of two phase

6513a35

fix unit test

184e05a

funky-eyes reviewed Dec 4, 2023

View reviewed changes

funky-eyes added this to the 2.1.0 milestone Dec 4, 2023

funky-eyes added type: bug Category issues or prs related to bug. module/spring spring module module/server server module module/core core module module/intergration-tx-api intergration-tx-api and removed module/spring spring module labels Dec 4, 2023

rollback tryotherapp param

27f1db4

rm Unused import

61df543

pengten reviewed Dec 5, 2023

View reviewed changes

optimize tcc judge

5efb325

slievrly requested changes Dec 6, 2023

View reviewed changes

1. rm try other app when send tc's order

988bb7a

funky-eyes approved these changes Dec 9, 2023

View reviewed changes

funky-eyes and others added 3 commits December 16, 2023 21:32

Merge branch '2.x' into 2.x

056c3f0

add the try other app back && allow at mode to enable

60b4adf

add try other app back

a6af930

funky-eyes requested a review from slievrly December 17, 2023 10:30

slievrly approved these changes Dec 17, 2023

View reviewed changes

funky-eyes merged commit 2f9957b into apache:2.x Dec 18, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: in dubbo 3.x version, the consumer can't generate tcc proxy. #6101

bugfix: in dubbo 3.x version, the consumer can't generate tcc proxy. #6101

laywin commented Dec 3, 2023 •

edited

Loading

codecov bot commented Dec 3, 2023 •

edited

Loading

funky-eyes Dec 4, 2023

laywin Dec 4, 2023

slievrly Dec 17, 2023

funky-eyes Dec 4, 2023

laywin Dec 4, 2023

funky-eyes Dec 4, 2023

slievrly Dec 4, 2023

laywin Dec 4, 2023

laywin Dec 4, 2023

pengten Dec 5, 2023

laywin Dec 5, 2023

pengten Dec 5, 2023

slievrly left a comment

funky-eyes commented Dec 7, 2023

funky-eyes left a comment

slievrly left a comment

bugfix: in dubbo 3.x version, the consumer can't generate tcc proxy. #6101

bugfix: in dubbo 3.x version, the consumer can't generate tcc proxy. #6101

Conversation

laywin commented Dec 3, 2023 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Dec 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slievrly left a comment

Choose a reason for hiding this comment

funky-eyes commented Dec 7, 2023

funky-eyes left a comment

Choose a reason for hiding this comment

slievrly left a comment

Choose a reason for hiding this comment

laywin commented Dec 3, 2023 •

edited

Loading

codecov bot commented Dec 3, 2023 •

edited

Loading