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: fix not return value when branchCommit and branchRollback throw exception #2755

Merged
merged 13 commits into from
Jun 5, 2020

Conversation

wangliang181230
Copy link
Contributor

@wangliang181230 wangliang181230 commented Jun 3, 2020

Ⅰ. Describe what this PR did

bugfix: TCC分支事务,在commit或rollback出现异常时,response不会写入分支事务数据的BUG修复。

此BUG会造成输出响应时抛出空指针异常。代码行:
io.seata.serializer.seata.protocol.transaction.AbstractBranchEndResponseCodec中的
55行 out.writeByte(branchStatus.getCode());

Ⅱ. 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

目前还遗留了一直异常时不断且快速重试的问题。需要新的重试机制的PR来解决此问题。

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2020

Codecov Report

Merging #2755 into develop will increase coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2755      +/-   ##
=============================================
+ Coverage      50.35%   50.49%   +0.13%     
  Complexity      2919     2919              
=============================================
  Files            572      572              
  Lines          18612    18613       +1     
  Branches        2212     2242      +30     
=============================================
+ Hits            9373     9398      +25     
+ Misses          8286     8285       -1     
+ Partials         953      930      -23     
Impacted Files Coverage Δ Complexity Δ
.../main/java/io/seata/rm/tcc/TCCResourceManager.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...a/server/storage/db/store/LogStoreDataBaseDAO.java 71.76% <0.00%> (+0.33%) 34.00% <0.00%> (ø%)
...ing/annotation/GlobalTransactionalInterceptor.java 24.60% <0.00%> (+0.60%) 7.00% <0.00%> (ø%)
...buf/convertor/BranchRollbackResponseConvertor.java 100.00% <0.00%> (+2.94%) 3.00% <0.00%> (ø%)
...tobuf/convertor/BranchCommitResponseConvertor.java 100.00% <0.00%> (+3.03%) 3.00% <0.00%> (ø%)
...buf/convertor/GlobalRollbackResponseConvertor.java 100.00% <0.00%> (+3.22%) 3.00% <0.00%> (ø%)
...tobuf/convertor/GlobalCommitResponseConvertor.java 100.00% <0.00%> (+3.33%) 3.00% <0.00%> (ø%)
...tobuf/convertor/GlobalStatusResponseConvertor.java 100.00% <0.00%> (+3.33%) 3.00% <0.00%> (ø%)
...rotobuf/convertor/RegisterRMResponseConvertor.java 82.75% <0.00%> (+3.44%) 3.00% <0.00%> (ø%)
...rotobuf/convertor/RegisterTMResponseConvertor.java 82.75% <0.00%> (+3.44%) 3.00% <0.00%> (ø%)
... and 14 more

@@ -52,7 +52,7 @@
out.writeShort((short)0);
}
out.writeLong(branchId);
out.writeByte(branchStatus.getCode());
out.writeByte((branchStatus == null ? BranchStatus.Unknown : branchStatus).getCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main problem is that what kind of situation would let the branch status null in branch commit or branch rollback.

Copy link
Contributor

Choose a reason for hiding this comment

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

because when branch commit or rollback, the branch status should not be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll find out why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the main problem is that what kind of situation would let the branch status null in branch commit or branch rollback.

I found the cause of the problem and fixed it.

@wangliang181230 wangliang181230 changed the title fixbug: NullPointerException when commit or rollback error. fixbug: NullPointerException when commit or rollback exception. Jun 4, 2020
@funky-eyes
Copy link
Contributor

does it break consistency without retrying?

@wangliang181230
Copy link
Contributor Author

does it break consistency without retrying?

嗯,我也考虑过。但是重试的频率太快了,如果同时有很多个事务都这样在重试,日志都要爆了。
目前重试机制相关的配置有哪些啊?

@funky-eyes
Copy link
Contributor

does it break consistency without retrying?

嗯,我也考虑过。但是重试的频率太快了,如果同时有很多个事务都这样在重试,日志都要爆了。
目前重试机制相关的配置有哪些啊?

我觉得你可以换个思路,给重试加上上限值,这个上限值可以通过配置中心获取.超过上限值后日志输出一次需要人工介入处理的日志.
还有就是你得找到为什么会出现重试的一个情况?脏读写还是什么问题?

@wangliang181230
Copy link
Contributor Author

does it break consistency without retrying?

嗯,我也考虑过。但是重试的频率太快了,如果同时有很多个事务都这样在重试,日志都要爆了。
目前重试机制相关的配置有哪些啊?

我觉得你可以换个思路,给重试加上上限值,这个上限值可以通过配置中心获取.超过上限值后日志输出一次需要人工介入处理的日志.
还有就是你得找到为什么会出现重试的一个情况?脏读写还是什么问题?

嗯,不断重试的原因肯定是由系统来找原因。只是我觉得seata最好也能提供一个重试的规则配置。
目前seata本身还没有这种重试上限机制是吧?也没有重试时间间隔随着重试次数的增加而延长的配置。

@funky-eyes
Copy link
Contributor

does it break consistency without retrying?

嗯,我也考虑过。但是重试的频率太快了,如果同时有很多个事务都这样在重试,日志都要爆了。
目前重试机制相关的配置有哪些啊?

我觉得你可以换个思路,给重试加上上限值,这个上限值可以通过配置中心获取.超过上限值后日志输出一次需要人工介入处理的日志.
还有就是你得找到为什么会出现重试的一个情况?脏读写还是什么问题?

嗯,不断重试的原因肯定是由系统来找原因。只是我觉得seata最好也能提供一个重试的规则配置。
目前seata本身还没有这种重试上限机制是吧?也没有重试时间间隔随着重试次数的增加而延长的配置。

我觉得你可以把这个pr转化为这个方向,加入一个重试的规则配置.原因的话seata一般有打印,比如脏读写的镜像数据与当前匹配不上.

@wangliang181230 wangliang181230 changed the title fixbug: NullPointerException when commit or rollback exception. fixbug: Response data not written when commit or rollback failed. Jun 4, 2020
@wangliang181230 wangliang181230 changed the title fixbug: Response data not written when commit or rollback failed. bugfix: Response data not written when commit or rollback failed. Jun 4, 2020
Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

Rollback fail retrying we should talk it in issue. Let us solve the problem that make the branch status null first. I already check the logic in

io.seata.rm.datasource.DataSourceManager#branchRollback
io.seata.rm.datasource.DataSourceManager#branchCommit

But look like the branch status would not be null.
Can you check these classes make the branch status null?

SagaResourceManager
TCCResourceManager
ResourceManagerXA
DataSourceManager

@wangliang181230
Copy link
Contributor Author

Rollback fail retrying we should talk it in issue. Let us solve the problem that make the branch status null first. I already check the logic in

io.seata.rm.datasource.DataSourceManager#branchRollback
io.seata.rm.datasource.DataSourceManager#branchCommit

But look like the branch status would not be null.
Can you check these classes make the branch status null?

SagaResourceManager
TCCResourceManager
ResourceManagerXA
DataSourceManager

RM和分支事务的状态并没有问题,而是出现异常时没有将状态写入BranchRollbackResponse中,从而导致AbstractBranchEndResponseCodec输出响应时,抛了空指针异常。

2)TCC的RM的二阶段不再往外抛异常,而是直接“return BranchStatus.PhaseTwo_XxxxxxFailed_Retryable;”.
@l81893521 l81893521 added the module/rm rm module label Jun 5, 2020
@wangliang181230 wangliang181230 changed the title bugfix: Response data not written when commit or rollback failed. bugfix: Response data not written when commit or rollback failed in tcc resource manager. Jun 5, 2020
Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

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

@wangliang181230
Copy link
Contributor Author

wangliang181230 commented Jun 5, 2020 via email

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

@slievrly slievrly changed the title bugfix: Response data not written when commit or rollback failed in tcc resource manager. bugfix: fix not return value when branchCommit and branchRollback throw exception Jun 5, 2020
@slievrly slievrly merged commit 711c2ec into apache:develop Jun 5, 2020
@wangliang181230 wangliang181230 deleted the fixbug-null branch June 8, 2020 01:55
@slievrly slievrly added this to the 1.3.0 milestone Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/rm rm module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants