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.

Sign up for GitHub

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

Jump to bottom

DefaultTransactionManager.begin fix #6105

Closed
wants to merge 2 commits into from
Closed

DefaultTransactionManager.begin fix #6105

wants to merge 2 commits into from

Conversation

lengweijian
Copy link

@lengweijian lengweijian commented Dec 6, 2023 •
edited
Loading

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

I find method name 'DefaultTransactionManager.begin' NPE occurred, when 'TM' or 'TR' registered failed !

Ⅱ. Does this pull request fix one issue?

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

only add one condition simply !

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2023 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ lengweijian
❌ [email protected]

[email protected] seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov Codecov
Copy link

codecov bot commented Dec 6, 2023 •
edited
Loading

Codecov Report

Merging #6105 (41158fc) into 2.x (9e5abc2) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6105      +/-   ##
============================================
- Coverage     49.45%   49.41%   -0.04%     
+ Complexity     4811     4805       -6     
============================================
  Files           913      913              
  Lines         31678    31678              
  Branches       3826     3826              
============================================
- Hits          15665    15653      -12     
- Misses        14474    14481       +7     
- Partials       1539     1544       +5
Files Coverage Δ
...in/java/io/seata/tm/DefaultTransactionManager.java 3.57% <0.00%> (ø)

... and 3 files with indirect coverage changes

@leizhiyuan leizhiyuan self-requested a review December 6, 2023 12:25
@leizhiyuan
Copy link
Contributor

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.1 out of 2 committers have signed the CLA.✅ lengweijian❌ [email protected]

[email protected] seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

you have two git name in commit log

@leizhiyuan leizhiyuan added the type: bug Category issues or prs related to bug. label Dec 6, 2023
leizhiyuan
leizhiyuan approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@leizhiyuan leizhiyuan 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 requested changes Dec 6, 2023
View reviewed changes
tm/src/main/java/io/seata/tm/DefaultTransactionManager.java
@@ -51,7 +52,7 @@ public String begin(String applicationId, String transactionServiceGroup, String
request.setTransactionName(name);
request.setTimeout(timeout);
GlobalBeginResponse response = (GlobalBeginResponse) syncCall(request);
if (response.getResultCode() == ResultCode.Failed) {
if (Objects.isNull(response) || response.getResultCode() == ResultCode.Failed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please submit relevant npe test reports

Copy link
Author

Choose a reason for hiding this comment

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

need to submit UT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Under what circumstances will NPE occur here?

Copy link
Author

Choose a reason for hiding this comment

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

when seats-sample project runs,, but not start seats-server.sh script, occurs!
image

Copy link
Contributor

Choose a reason for hiding this comment

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

问题的根因不在这里，请你把完整堆栈发出来
The root cause of the problem is not here, please send out the complete stack

Copy link
Contributor

Choose a reason for hiding this comment

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

而且堆栈是54行，你修改的是55行，你应该提交一个issue，而不是一个pr，一个记录完整异常描述和seata版本的issue

Moreover, the stack trace is at line 54 and you made a modification at line 55. You should submit an issue instead of a pull request, which includes the complete description of the exception and the version of Seata.

Copy link
Member

@slievrly slievrly Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

NPE may occur in earlier versions due to request timeout, please submit an issue as detailed in the template.

Copy link
Author

Choose a reason for hiding this comment

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

ok

@funky-eyes funky-eyes added Do Not Merge Do not merge into develop and removed type: bug Category issues or prs related to bug. labels Dec 7, 2023
@funky-eyes funky-eyes closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slievrly slievrly slievrly left review comments

@funky-eyes funky-eyes funky-eyes requested changes

@leizhiyuan leizhiyuan leizhiyuan approved these changes

Assignees
No one assigned
Labels
Do Not Merge Do not merge into develop
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lengweijian @CLAassistant @leizhiyuan @slievrly @funky-eyes