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: can't post TimeoutRollbacked event #4780

Merged
merged 20 commits into from Oct 20, 2022
Merged

bugfix: can't post TimeoutRollbacked event #4780

merged 20 commits into from Oct 20, 2022

Conversation

tuwenlin
Copy link
Contributor

当出现超时回滚时,由timeoutCheck线程把事务状态修改为TimeoutRollbacking,随后由handleRetryRollbacking去处理回滚,而在handleRetryRollbacking中的回滚,retry的值是true,在SessionHelper中,如果是非file模式,在endRollbacked方法中走不到else逻辑,不会发出TimeoutRollbacked事件,且在retryGlobal为true的逻辑中,直接在GlobalSession被remove后发出了AfterRollbacked事件,也没有发出TimeoutRollbacked事件

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022

Codecov Report

Merging #4780 (c150b93) into develop (caeb231) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4780   +/-   ##
==========================================
  Coverage      48.62%   48.62%           
- Complexity      4052     4053    +1     
==========================================
  Files            732      732           
  Lines          25773    25775    +2     
  Branches        3173     3173           
==========================================
+ Hits           12532    12534    +2     
+ Misses         11909    11908    -1     
- Partials        1332     1333    +1     
Impacted Files Coverage Δ
...o/seata/server/coordinator/DefaultCoordinator.java 44.48% <0.00%> (ø)
...in/java/io/seata/server/session/SessionHelper.java 65.62% <66.66%> (+0.73%) ⬆️
...n/src/main/java/io/seata/common/util/IdWorker.java 77.08% <0.00%> (-6.25%) ⬇️
...rage/redis/store/RedisTransactionStoreManager.java 67.52% <0.00%> (+0.56%) ⬆️
...very/registry/zk/ZookeeperRegisterServiceImpl.java 61.76% <0.00%> (+0.73%) ⬆️

@funky-eyes funky-eyes added this to the 1.6.0 milestone Jul 15, 2022
@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. module/server server module labels Jul 15, 2022
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 changed the title bugfix:can‘t post TimeoutRollbacked event bugfix: can‘t post TimeoutRollbacked event Jul 16, 2022
@funky-eyes funky-eyes requested a review from slievrly July 16, 2022 05:02
@@ -331,6 +332,8 @@ protected void timeoutCheck() {

// transaction timeout and start rollbacking event
MetricsPublisher.postSessionDoingEvent(globalSession, GlobalStatus.TimeoutRollbacking.name(), false, false);
// do rollback
core.doGlobalRollback(globalSession, false);
Copy link
Member

Choose a reason for hiding this comment

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

timeoutCheck as mark thread, don't add time-consuming operations to it.

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 have moved the operation to 'retryRollbacking' thread and change the status to 'Rollbacking' after the first successful rollback

@@ -180,6 +180,7 @@ public static void endRollbacked(GlobalSession globalSession, boolean retryGloba
beginTime, retryBranch);
} else {
if (SessionStatusValidator.isTimeoutGlobalStatus(globalSession.getStatus())) {
globalSession.changeGlobalStatus(GlobalStatus.Rollbacking);
Copy link
Contributor

Choose a reason for hiding this comment

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

已经是rollbacking status的是否会进这里?如果会是否存在重复修改?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

进到这个满足几个条件,retry是false,且stauts是TimeoutRollbacked,TimeoutRollbackFailed,TimeoutRollbacking,TimeoutRollbackRetrying,也就是说,只有因为超时回滚的相关status且非重试才会走到这,只有TimeoutRollbacking的情况下,retry才是false,其他情况下都是true(至少我没发现其他超时相关status且retry是false的情况,DefaultCoordinator的382行做了限制),应该不会存在重复修改

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 remove the globalSession(status=TimeoutRollbacking)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

在SessionHelper的183行,TimeoutRollbacking的session被改成了Rollbacking,2分30秒后被handleRetryCommitting删掉

Copy link
Member

Choose a reason for hiding this comment

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

Is the flow of transaction state reasonable?

TimeoutRollbacking -> Rollbacking Because you want to delete it?

changes/en-us/develop.md Outdated Show resolved Hide resolved
changes/zh-cn/develop.md Outdated Show resolved Hide resolved
@caohdgege caohdgege changed the title bugfix: can‘t post TimeoutRollbacked event bugfix: can't post TimeoutRollbacked event Jul 27, 2022
@xingfudeshi xingfudeshi self-requested a review October 19, 2022 08:01
Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

LGTM

@xingfudeshi xingfudeshi merged commit f56b02c into apache:develop Oct 20, 2022
} else {
MetricsPublisher.postSessionDoneEvent(globalSession, GlobalStatus.Rollbacked, false, false);
}
MetricsPublisher.postSessionDoneEvent(globalSession, GlobalStatus.Rollbacked, false, false);
Copy link
Member

Choose a reason for hiding this comment

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

comment: retryGlobal of TimeoutXXX must be true.
if status=TimeoutRollbacked there is no need to print the metric again
if status=TimeoutRollbackFailed will call endRollbackFailed
if status=TimeoutRollbackRetrying will step into https://github.com/seata/seata/pull/4780/files#diff-9a05c8e25f5cbde274749846da97233df30a162a9dd93078d4e41738a948a09eR165

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/server server 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

6 participants