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 wrong exception information when rollback fails due to dirty data #2333

Merged
merged 22 commits into from
Mar 7, 2020

Conversation

funky-eyes
Copy link
Contributor

Ⅰ. Describe what this PR did

fix wrong exception information when rollback fails due to dirty data

Ⅱ. Does this pull request fix one issue?

#2325

@codecov-io
Copy link

codecov-io commented Mar 2, 2020

Codecov Report

Merging #2333 into develop will decrease coverage by 0.17%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2333      +/-   ##
============================================
- Coverage      51.77%   51.6%   -0.18%     
+ Complexity      2698    2695       -3     
============================================
  Files            517     517              
  Lines          16752   16740      -12     
  Branches        2033    1995      -38     
============================================
- Hits            8674    8639      -35     
+ Misses          7270    7268       -2     
- Partials         808     833      +25
Impacted Files Coverage Δ Complexity Δ
...ing/annotation/GlobalTransactionalInterceptor.java 9.72% <0%> (-0.14%) 2 <0> (ø)
...ava/io/seata/tm/api/DefaultFailureHandlerImpl.java 68.57% <0%> (-6.43%) 6 <0> (ø)
...in/java/io/seata/tm/api/TransactionalExecutor.java 62.96% <100%> (+1.42%) 0 <0> (ø) ⬇️
...in/java/io/seata/tm/api/TransactionalTemplate.java 66.66% <50%> (-0.63%) 24 <0> (ø)
...obuf/convertor/BranchRegisterRequestConvertor.java 90.47% <0%> (-9.53%) 3% <0%> (ø)
...otobuf/convertor/BranchReportRequestConvertor.java 90.9% <0%> (-9.1%) 3% <0%> (ø)
...obuf/convertor/BranchRollbackRequestConvertor.java 92% <0%> (-8%) 3% <0%> (ø)
...otobuf/convertor/GlobalBeginResponseConvertor.java 92.59% <0%> (-7.41%) 3% <0%> (ø)
...otobuf/convertor/GlobalStatusRequestConvertor.java 93.75% <0%> (-6.25%) 3% <0%> (ø)
...otobuf/convertor/GlobalCommitRequestConvertor.java 93.75% <0%> (-6.25%) 3% <0%> (ø)
... and 24 more

@@ -143,7 +144,9 @@ private void rollbackTransaction(GlobalTransaction tx, Throwable ex) throws Tran
tx.rollback();
triggerAfterRollback();
// 3.1 Successfully rolled back
throw new TransactionalExecutor.ExecutionException(tx, TransactionalExecutor.Code.RollbackDone, ex);
throw new TransactionalExecutor.ExecutionException(tx,
Copy link
Member

Choose a reason for hiding this comment

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

I feel that the code you changed is not correct. I think it is OK to throw RollbackDone directly here.

@slievrly
Copy link
Member

slievrly commented Mar 2, 2020

@a364176773 check the code, no problem. However, there are a few minor problems. Tx.getStatus () is a remote call, but in fact status is already available when rollback. The second is that the exception thrown by tx.rollback () must be the rpc exception of the framework and the business rollback fails Both are defined with RollbackFailure, without distinction.

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, pr title should be written according to the specification.

@slievrly
Copy link
Member

slievrly commented Mar 3, 2020

@a364176773 Please solve the problem of getStatus to get the remote status, in fact status has been returned in rollback.

@funky-eyes
Copy link
Contributor Author

@a364176773 Please solve the problem of getStatus to get the remote status, in fact status has been returned in rollback.

@slievrly i will submit in additional

@funky-eyes funky-eyes changed the title fix wrong exception information when rollback fails due to dirty data bugfix:fix wrong exception information when rollback fails due to dirty data Mar 4, 2020
@zjinlei zjinlei self-assigned this Mar 5, 2020
@apache apache deleted a comment from jovany-wang Mar 5, 2020
@funky-eyes
Copy link
Contributor Author

@zjinlei @xingfudeshi PTAL

@@ -67,6 +67,12 @@ public void onRollbackFailure(GlobalTransaction tx, Throwable cause) {
timer.newTimeout(new CheckTimerTask(tx, GlobalStatus.Rollbacked), SCHEDULE_INTERVAL_SECONDS, TimeUnit.SECONDS);
}

@Override
public void onRollbackRetrying(GlobalTransaction tx, Throwable cause) {
LOGGER.warn("Failed to rollback transaction[" + tx.getXid() + "]", cause);
Copy link
Member

Choose a reason for hiding this comment

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

Please use {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@funky-eyes
Copy link
Contributor Author

@xingfudeshi @zjinlei PTAL

optimize: adjust the processing logic for unsupported listeners (apache#2354)
@Override
public void onRollbackRetrying(GlobalTransaction tx, Throwable cause) {
StackTraceLogger.info(LOGGER, cause, "Retrying to rollback transaction[{}]", new String[] {tx.getXid()}, null,
null);
Copy link
Contributor

Choose a reason for hiding this comment

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

StackTraceLogger.info(LOGGER, cause, "Retrying to rollback transaction[{}]", new String[] {tx.getXid()}, "Retrying to rollback transaction[{}]", new String[] {tx.getXid()});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StackTraceLogger.info(LOGGER, cause, "Retrying to rollback transaction[{}]", new String[] {tx.getXid()}, "Retrying to rollback transaction[{}]", new String[] {tx.getXid()});

PTAL @zjinlei

Copy link
Contributor

@zjinlei zjinlei left a comment

Choose a reason for hiding this comment

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

LGTM

@zjinlei zjinlei merged commit 21e0355 into apache:develop Mar 7, 2020
@slievrly slievrly added this to the 1.2.0 milestone Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants