Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize: branch register resource only at RM server end #5399

Merged
merged 44 commits into from
Jan 8, 2024

Conversation

sunrui1225
Copy link
Contributor

@sunrui1225 sunrui1225 commented Mar 1, 2023

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

optimize fence prepare from tm to rm.
At rm, only enhence the native bean, exclude all of proxy bean.

Ⅱ. Does this pull request fix one issue?

fiex #5596

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@slievrly slievrly self-assigned this Mar 4, 2023
@Bughue
Copy link
Contributor

Bughue commented Mar 20, 2023

没有关联issue吗

}

// eliminate aop proxy.
if (AopUtils.isAopProxy(target)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past, the result of being processed by the RPC framework would have been either a Reference Bean or a Service Bean. However, if you are not currently using this RPC framework handling, how do you ensure that the class is not proxy or that the TwoPhaseBusinessAction has the highest priority order?


在过去,被RPC框架处理后,最终得到的结果肯定是Reference Bean或者Service Bean。但是,如果当前不再使用这种RPC框架的处理方式来判断,如何保证这个类没有被代理过或者怎么保证TwoPhaseBusinessAction的切面优先级是最高的?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

之前这种方式确实有问题,不能保证,以下以一种思路:
利用spring 的 bean definition,提前在自动代理前得到各个类的引用关系,从而提前获取依赖关系,保存名字在Set中,然后在AOP自动代理的wrapIfNecessary中,判断是否存在。
在RM端,Set中保存的是 service指向的原生bean的name。
在TM端,Set中保存的是开启事务的bean的name。

private Set<String> tccProxyTargetMethod(RemotingDesc remotingDesc) {
if (!remotingDesc.isReference() || remotingDesc == null) {

private Set<String> tccProxyTargetMethod(Object target) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to handle method overloading with the same method name


如何处理方法名相同都标注TwoPhaseBusinessAction注解的方法重载问题。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to handle method overloading with the same method name

如何处理方法名相同都标注TwoPhaseBusinessAction注解的方法重载问题。

这个问题将由后续pr解决
This issue will be addressed by a follow-up PR

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2023

Codecov Report

Merging #5399 (1da082e) into 2.x (501d431) will decrease coverage by 0.12%.
The diff coverage is 30.64%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5399      +/-   ##
============================================
- Coverage     48.98%   48.87%   -0.12%     
+ Complexity     4782     4781       -1     
============================================
  Files           913      915       +2     
  Lines         31697    31762      +65     
  Branches       3823     3829       +6     
============================================
- Hits          15528    15523       -5     
- Misses        14632    14709      +77     
+ Partials       1537     1530       -7     
Files Coverage Δ
...ing/remoting/parser/RemotingFactoryBeanParser.java 0.00% <0.00%> (ø)
...m/tcc/interceptor/TccActionInterceptorHandler.java 64.40% <75.00%> (ø)
...rm/tcc/remoting/parser/LocalTCCRemotingParser.java 50.00% <50.00%> (ø)
...on/tx/api/remoting/parser/DubboRemotingParser.java 0.00% <0.00%> (ø)
...tion/tx/api/remoting/parser/HSFRemotingParser.java 0.00% <0.00%> (ø)
.../tx/api/remoting/parser/SofaRpcRemotingParser.java 0.00% <0.00%> (ø)
...ion/tx/api/interceptor/parser/NeedEnhanceEnum.java 0.00% <0.00%> (ø)
...tcc/resource/parser/TccRegisterResourceParser.java 91.48% <87.50%> (+6.58%) ⬆️
.../tx/api/remoting/parser/DefaultRemotingParser.java 0.00% <0.00%> (ø)
...api/interceptor/parser/DefaultInterfaceParser.java 50.00% <0.00%> (-18.75%) ⬇️
... and 5 more

... and 4 files with indirect coverage changes

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2023

Codecov Report

Attention: 88 lines in your changes are missing coverage. Please review.

Comparison is base (26feac2) 50.00% compared to head (5af4071) 49.84%.
Report is 1 commits behind head on 2.x.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5399      +/-   ##
============================================
- Coverage     50.00%   49.84%   -0.16%     
+ Complexity     4932     4930       -2     
============================================
  Files           915      917       +2     
  Lines         31927    31995      +68     
  Branches       3855     3862       +7     
============================================
- Hits          15966    15949      -17     
- Misses        14400    14486      +86     
+ Partials       1561     1560       -1     
Files Coverage Δ
...va/io/seata/integration/tx/api/util/ProxyUtil.java 66.66% <ø> (ø)
...ing/remoting/parser/RemotingFactoryBeanParser.java 0.00% <0.00%> (ø)
...m/tcc/interceptor/TccActionInterceptorHandler.java 64.40% <75.00%> (ø)
...rm/tcc/remoting/parser/LocalTCCRemotingParser.java 50.00% <50.00%> (ø)
...on/tx/api/remoting/parser/DubboRemotingParser.java 0.00% <0.00%> (ø)
...tion/tx/api/remoting/parser/HSFRemotingParser.java 0.00% <0.00%> (ø)
.../tx/api/remoting/parser/SofaRpcRemotingParser.java 0.00% <0.00%> (ø)
...ion/tx/api/interceptor/parser/NeedEnhanceEnum.java 0.00% <0.00%> (ø)
...tcc/resource/parser/TccRegisterResourceParser.java 91.48% <87.50%> (+6.58%) ⬆️
.../tx/api/remoting/parser/DefaultRemotingParser.java 0.00% <0.00%> (ø)
... and 6 more

... and 5 files with indirect coverage changes

# Conflicts:
#	integration-tx-api/src/main/java/io/seata/integration/tx/api/interceptor/parser/DefaultInterfaceParser.java
@funky-eyes funky-eyes changed the title Resolve fence to rm optimize: resolve fence to rm Dec 23, 2023
@sunrui1225 sunrui1225 changed the title optimize: resolve fence to rm optimizing branch register resource only at RM server end Dec 23, 2023
@funky-eyes funky-eyes changed the title optimizing branch register resource only at RM server end optimize: branch register resource only at RM server end Dec 23, 2023
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image
经过测试符合预期,但是如果在使用tcc时,在prepare上增加globaltransactional注解时,会使TwoPhaseBusinessAction注解失效,当然这个应该是另一个问题,应该后续进行关注
Tested as expected, but if you use tcc, adding globaltransactional annotations to prepare will invalidate TwoPhaseBusinessAction annotations. Of course, this should be another issue and should be followed up

funky-eyes
funky-eyes previously approved these changes Jan 2, 2024
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (ifNeedEnhanceBean.getNeedEnhanceEnum().equals(NeedEnhanceEnum.SERVICE_BEAN)) {
// the native bean which dubbo, sofa bean service bean referenced
PropertyValue propertyValue = beanDefinition.getPropertyValues().getPropertyValue("ref");
if (propertyValue == null) {
Copy link
Contributor

@Bughue Bughue Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这些逻辑看起来应该和parser强相关,是不是该挪到parser去?虽然这样做会带来一部分重复的代码
This logic seems like it should be strongly related to the parser, so should it be moved to the parser, even though it would be a duplicate piece of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果引入到parser中,可能会给parser带来beanDefinition的spring依赖,原则上可能不想让parser层引入spring相关的依赖


private NeedEnhanceEnum needEnhanceEnum;

public boolean isIfNeed() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is need/isNeed() better than ifNeed/isIfNeed() ?

funky-eyes
funky-eyes previously approved these changes Jan 4, 2024
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wt-better
Copy link
Contributor

LGTM

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@funky-eyes funky-eyes merged commit 1679849 into apache:2.x Jan 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mode: TCC TCC transaction mode module/spring spring module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants