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

bugfix: global transaction hook repeat execute #5887

Merged
merged 13 commits into from
Nov 11, 2023
Merged

Conversation

jsbxyyx
Copy link
Member

@jsbxyyx jsbxyyx commented Sep 27, 2023

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes #5595

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #5887 (7be0b69) into develop (613959a) will decrease coverage by 0.04%.
The diff coverage is 55.00%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5887      +/-   ##
=============================================
- Coverage      48.90%   48.87%   -0.04%     
  Complexity      4185     4185              
=============================================
  Files            793      793              
  Lines          28015    28028      +13     
  Branches        3418     3423       +5     
=============================================
- Hits           13701    13698       -3     
- Misses         12889    12900      +11     
- Partials        1425     1430       +5     
Files Coverage Δ
...in/java/io/seata/tm/api/TransactionalTemplate.java 55.05% <55.00%> (-1.31%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM
add change

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

@jsbxyyx Pls. check the UT.

} catch (Exception e) {
LOGGER.error("Failed execute afterCompletion in hook {}", e.getMessage(), e);
private void triggerAfterCompletion(GlobalTransaction tx) {
if (tx == null || tx.getGlobalTransactionRole() == GlobalTransactionRole.Launcher) {
Copy link
Contributor

Choose a reason for hiding this comment

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

which scene tx can be null ? it seems tx can not null from the invoke context

Copy link
Member Author

Choose a reason for hiding this comment

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

这里不会为null,从事务模型上来说,事务开启失败了,是否需要调用completion

Copy link
Contributor

Choose a reason for hiding this comment

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

我看其他的hook判断条件里面,都认为tx不会为null,所以都没有这个tx==null的判断,但是triggerAfterCompletion 这个需要有这个特殊处理。后面修改的人可能会疑惑

如果认为这里tx 也不会为null,是不是可以吧tx==null的条件去掉,和其他修改的地方保持一致?

I see in the conditions of other hook judgments that they all assume tx will not be null, so there is no tx==null condition. However, triggerAfterCompletion requires this special handling. The subsequent modifications may be confusing to people.

If we also assume that tx will not be null here, can we remove the tx==null condition and keep it consistent with other modifications?

Copy link
Member Author

Choose a reason for hiding this comment

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

这里的判断是和saga保持一致的

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.

The hook is called by TransactionalTemplate, and the Participant's judgment is inside the class defaultGlobaltransaction, so I'm wondering again if I should underlay the hook call into defaultGlobaltransaction? Or the other way around, make defaultGlobaltransaction's behavior more pure, and all the prevention of duplicate calls and permission control is handled by TransactionalTemplate?

钩子是TransactionalTemplate调的,而Participant的判断是在defaultGlobaltransaction这个类里面的,所以我再想是否应该将hook的调用下层到defaultGlobaltransaction中?或者反过来,让defaultGlobaltransaction的行为更加单纯,所有的防止重复调用,权限控制都由TransactionalTemplate来处理?

@jsbxyyx
Copy link
Member Author

jsbxyyx commented Oct 31, 2023

The hook is called by TransactionalTemplate, and the Participant's judgment is inside the class defaultGlobaltransaction, so I'm wondering again if I should underlay the hook call into defaultGlobaltransaction? Or the other way around, make defaultGlobaltransaction's behavior more pure, and all the prevention of duplicate calls and permission control is handled by TransactionalTemplate?

钩子是TransactionalTemplate调的,而Participant的判断是在defaultGlobaltransaction这个类里面的,所以我再想是否应该将hook的调用下层到defaultGlobaltransaction中?或者反过来,让defaultGlobaltransaction的行为更加单纯,所有的防止重复调用,权限控制都由TransactionalTemplate来处理?

角色判断修改为提前处理

@funky-eyes funky-eyes modified the milestones: 2.0.0, 1.8.1 Nov 4, 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.

LGTM

@funky-eyes
Copy link
Contributor

image
Unit tests need to be fixed before merging can take place

@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. module/tm tm module labels Nov 4, 2023
@funky-eyes funky-eyes merged commit 21af1bc into apache:develop Nov 11, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/tm tm module type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants