feature: Implement scheduled handling for end status transaction #7133

YongGoose · 2025-01-31T05:39:15Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

Add compensation handling for TimeoutRollbacked and Rollbacked states

Ⅱ. Does this pull request fix one issue?

fixes #6875

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

…tates

funky-eyes · 2025-02-05T01:31:56Z

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

+        GlobalStatus.TimeoutRollbacked,
+        GlobalStatus.Rollbacked


由于rollback的概率是非常小的，并且rollbacked和TimeoutRollbacked的状态残留的概率更低，故这两种状态是特意放置在server重启时进行处理。
Since the probability of rollback is very low, and the chances of the rollbacked and TimeoutRollbacked states persisting are even lower, these two states are specifically handled during server restart.

由于rollback的概率是非常小的，并且rollbacked和TimeoutRollbacked的状态残留的概率更低，故这两种状态是特意放置在server重启时进行处理。 Since the probability of rollback is very low, and the chances of the rollbacked and TimeoutRollbacked states persisting are even lower, these two states are specifically handled during server restart.

Thank you for your comment :)

I didn’t fully understand.

Currently, in DefaultCoordinator, there is compensation handling for the committed state, but there is no compensation handling for the TimeoutRollbacked and Rollbacked states.

Based on the comment above, I added these two cases to the array for management.
If any changes are needed, could you please provide additional explanations?

Since the probability of a transaction rollback occurring is very low, the likelihood of the rollbacked intermediate state persisting is even lower. After the rollbacking state, it transitions to rollbacked and is then deleted. These two actions are separated by just one I/O operation, meaning that once the status is successfully updated to rollbacked, the transaction will be immediately deleted, unless a network or storage-side exception occurs during deletion, preventing the transaction from being deleted. Otherwise, there will be no rollbacked transactions left.
The commit of a transaction is a high-probability event, so the likelihood of the committed state persisting is much higher than that of rollbacked. To prevent storage-side pressure and meet performance requirements, committed transactions need to be processed during the server's runtime, while the low-probability rollbacked transactions are handled during the server startup.
Since both of these probabilities are very low, the final result is that they are both deleted. If you think it is necessary to handle rollbacked transactions during runtime, I suggest that you create a separate scheduled task to batch process transactions in the rollbacked and committed states, rather than blocking transactions that urgently need to be rolled back and retried.

If you think it is necessary to handle rollbacked transactions during runtime, I suggest that you create a separate scheduled task to batch process transactions in the rollbacked and committed states, rather than blocking transactions that urgently need to be rolled back and retried.

Thank you for your suggestion!

The idea you proposed is very interesting, and I’m planning to work on it. :)

Additionally, I’ve posted a message on the Seata project’s issue list and mailing list regarding this topic.
I’ll leave the links below—could you kindly take a look when you have a chance?

https://lists.apache.org/thread/xlkshkqs5v2hflp10jgz418kos8vwrsc

optimize seata-client TM&RM netty eventloop #6774

Thanks again! 🚀

@funky-eyes

Done in fc28796 !
PTAL ⚡️

YongGoose · 2025-02-05T02:44:54Z

@funky-eyes

In the issue description, it was mentioned that the state remains unchanged unless the server is restarted.
This means that the data will persist unless the server is restarted.

So, aside from handling it specifically during a server restart, I believe additional measures should be taken.

WDYT?

funky-eyes · 2025-02-07T01:27:40Z

Do you use the DingTalk app?

YongGoose · 2025-02-07T01:33:33Z

Do you use the DingTalk app?

Unfortunately, no..

However, if using DingTalk allows for better communication, I’m willing to install it!

funky-eyes · 2025-02-07T01:50:02Z

Do you use the DingTalk app?

Unfortunately, no..

However, if using DingTalk allows for better communication, I’m willing to install it!

Of course, email is also a great communication channel, but I would like to invite you to join the Seata contributors' DingTalk group if you're willing to install it. If you install it, please send your DingTalk ID to my email, and I will invite you to join the Seata contributors' group.

codecov · 2025-02-08T06:16:26Z

Codecov Report

Attention: Patch coverage is 34.54545% with 36 lines in your changes missing coverage. Please review.

Project coverage is 52.17%. Comparing base (2c31bdd) to head (bd9f7dd).
Report is 1 commits behind head on 2.x.

Files with missing lines	Patch %	Lines
...e/seata/server/coordinator/DefaultCoordinator.java	34.21%	23 Missing and 2 partials ⚠️
...org/apache/seata/server/session/SessionHelper.java	0.00%	7 Missing ⚠️
...org/apache/seata/server/session/GlobalSession.java	33.33%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #7133      +/-   ##
============================================
- Coverage     52.20%   52.17%   -0.03%     
- Complexity     6808     6811       +3     
============================================
  Files          1154     1154              
  Lines         41058    41112      +54     
  Branches       4813     4818       +5     
============================================
+ Hits          21434    21451      +17     
- Misses        17585    17621      +36     
- Partials       2039     2040       +1

Files with missing lines	Coverage Δ
...ava/org/apache/seata/common/ConfigurationKeys.java	`0.00% <ø> (ø)`
...c/main/java/org/apache/seata/common/Constants.java	`100.00% <ø> (ø)`
...in/java/org/apache/seata/common/DefaultValues.java	`0.00% <ø> (ø)`
...oconfigure/properties/server/ServerProperties.java	`100.00% <100.00%> (ø)`
...org/apache/seata/server/session/GlobalSession.java	`68.32% <33.33%> (-0.77%)`	⬇️
...org/apache/seata/server/session/SessionHelper.java	`46.10% <0.00%> (-2.02%)`	⬇️
...e/seata/server/coordinator/DefaultCoordinator.java	`40.05% <34.21%> (-0.86%)`	⬇️

... and 5 files with indirect coverage changes

funky-eyes · 2025-02-13T01:25:55Z

server/src/main/java/org/apache/seata/server/session/GlobalSession.java

     * @return time to dead session. if not greater than 0, then deadSession
     */
    public long timeToDeadSession() {
+        if (this.status == GlobalStatus.Rollbacked || this.status == GlobalStatus.TimeoutRollbacked) {
+            return beginTime - System.currentTimeMillis();


The beginTime does not change after the creation of the global session, which will cause any global session to be added to the needDoRollbackedSessions list.

I implemented it this way to set a slightly shorter interval for sessions in the end state.

If possible, could you please provide a more detailed explanation? 🙂

The beginTime is set when the transaction is created. When the Seata server's business thread changes the transaction status from "Rollbacking" to "Rollbacked," your timeToDeadSession method directly subtracts the current timestamp from beginTime, which will inevitably result in a negative value. As a result, the business thread will delete the transaction, and your scheduled task will also delete the transaction, leading to a concurrency issue and thread safety problem. I believe that beginTime should be increased by the corresponding transaction timeout, plus an additional buffer time to ensure thread safety.

Then, would it be appropriate to create a RETRY_DEAD_THRESHOLD specifically for the end state?

Of course, this timeToDeadSession method is more relevant for ongoing statuses like "rollbacking" or "committing." For transactions in the "end" status, I believe the interval can be shortened.

As suggested in the discussion, since we can reduce the interval in the end state, I believe decreasing the size of RETRY_DEAD_THRESHOLD could be a reasonable approach.

The beginTime is set when the transaction is created. When the Seata server's business thread changes the transaction status from "Rollbacking" to "Rollbacked," your timeToDeadSession method directly subtracts the current timestamp from beginTime, which will inevitably result in a negative value. As a result, the business thread will delete the transaction, and your scheduled task will also delete the transaction, leading to a concurrency issue and thread safety problem. I believe that beginTime should be increased by the corresponding transaction timeout, plus an additional buffer time to ensure thread safety.

Since our comments were written at the same time, I didn’t see yours when I was writing mine. 😅

Thank you for the additional explanation!
In that case, I will adjust the execution interval for sessions in the end state to be shorter using an if condition.

RETRY_DEAD_THRESHOLD

I think this can be done this way

RETRY_DEAD_THRESHOLD

I think this can be done this way

Then no changes required?

Then, would it be appropriate to create a RETRY_DEAD_THRESHOLD specifically for the end state?

or you mean this way?

Create a corresponding RETRY_DEAD_THRESHOLD for the end state

Done in 997775c !

PTAL ⚡️

funky-eyes · 2025-02-13T01:28:56Z

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java


    private final GlobalStatus[] retryCommittingStatuses = new GlobalStatus[] {GlobalStatus.CommitRetrying, GlobalStatus.Committed};

    private final GlobalStatus[] rollbackingStatuses = new GlobalStatus[] {GlobalStatus.Rollbacking};
    private final GlobalStatus[] committingStatuses = new GlobalStatus[] {GlobalStatus.Committing};
+    private final GlobalStatus[] rollbackedStatuses = new GlobalStatus[] {GlobalStatus.Rollbacked, GlobalStatus.TimeoutRollbacked};


Why not process transactions in other end states together?

I will include the CommitFailed, RollbackFailed, TimeoutRollbackFailed, Finished, CommitRetryTimeout, and RollbackRetryTimeout states :)

Done in a9ec30f !

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

funky-eyes · 2025-02-15T09:52:34Z

common/src/main/java/org/apache/seata/common/ConfigurationKeys.java

+    /**
+     * The constant ROLLBACKED_RETRY_PERIOD.
+     */
+    String ROLLBACKED_RETRY_PERIOD = RECOVERY_PREFIX + "rollbackedRetryPeriod";


https://github.com/apache/incubator-seata/blob/2.x/script/config-center/config.txt
https://github.com/apache/incubator-seata/blob/2.x/server/src/main/resources/application.example.yml
https://github.com/apache/incubator-seata/blob/2.x/server/src/main/resources/application.raft.example.yml
https://github.com/apache/incubator-seata/blob/2.x/seata-spring-autoconfigure/seata-spring-autoconfigure-server/src/main/java/org/apache/seata/spring/boot/autoconfigure/properties/server/ServerProperties.java
This configuration item needs to be supplemented in the above file, and I think it is no longer appropriate to name this configuration starting with rollback, because it is a configuration name for handling transactions in the end state..

Done in 932c51f !

server/src/main/java/org/apache/seata/server/session/GlobalSession.java

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

funky-eyes · 2025-02-15T09:57:44Z

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

+
+                    // commit retry timeout event
+                    SessionHelper.endRollbackFailed(rollbackedSession, true, true);
+
+                    //The function of this 'return' is 'continue'.
+                    return;
+                }
+                core.doGlobalRollback(rollbackedSession, true);


It should no longer perform rollback processing. For different states of transactions in the end phase, call the SessionHelper.endRollbacked and SessionHelper.endCommitted methods respectively.

Done in a7a9e52 !

@funky-eyes

When you have time, I would appreciate it if you could review this comment and the issue below.

optimize seata-client TM&RM netty eventloop #6774

…sion.java Co-authored-by: funkye <[email protected]>

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

changes/zh-cn/2.x.md

common/src/main/java/org/apache/seata/common/DefaultValues.java

Co-authored-by: funkye <[email protected]>

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

funky-eyes · 2025-02-17T01:37:15Z

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

+    private void handleEndStateSession(GlobalSession globalSession) throws TransactionException {
+        GlobalStatus status = globalSession.getStatus();
+
+        if (status == GlobalStatus.CommitFailed) {


The commit state is not limited to just one, and the rollback state is the same. I suggest putting the handling of various end states directly in the SessionHelper, and then calling it from handleEndStateSession. The only input parameter needed is a globalsession

As I understand it, instead of directly managing the transaction end states with conditional statements in handleEndStateSession, the responsibility is being delegated to SessionHelper.

Is my understanding correct?

Below is the implementation based on my understanding.

private void handleEndStateSession(GlobalSession globalSession) throws TransactionException { SessionHelper.processEndState(globalSession); }

The state values are just examples.

public class SessionHelper { public static void processEndState(GlobalSession globalSession) throws TransactionException { GlobalStatus status = globalSession.getStatus(); switch (status) { case Committed: case CommitSuccess: endCommitted(globalSession, false); break; case CommitFailed: case CommitPending: endCommitted(globalSession, true); break; case Rollbacked: case RollbackSuccess: endRollbacked(globalSession, false); break; case RollbackFailed: case RollbackPending: endRollbacked(globalSession, true); break; default: log.warn("Unknown transaction state: {}", status); } } }

Yes, I think it should be this way, encapsulating the processing logic in the SessionHelper to hide the internal implementation from the outside.

Done in 5837ae6 !

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

YongGoose · 2025-02-17T09:58:57Z

@funky-eyes

You can see in the comment that the word rollbacked has been changed to status. 113a7fd

Please take a look and provide your feedback.

funky-eyes

I believe the committed state should be handled within the end state task, and transactions in a failed state should not be retried. Failed transactions need to be addressed by the operations team, rather than being handled by scheduled tasks.

server/src/main/java/org/apache/seata/server/session/SessionHelper.java

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

…lper.java Co-authored-by: funkye <[email protected]>

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

…lper.java Co-authored-by: funkye <[email protected]>

server/src/main/java/org/apache/seata/server/session/GlobalSession.java

YongGoose · 2025-02-18T01:43:45Z

I believe the committed state should be handled within the end state task, and transactions in a failed state should not be retried. Failed transactions need to be addressed by the operations team, rather than being handled by scheduled tasks.

That sounds like a good point.

However, while going through your comment, a few questions came to mind:

Could you explain the situations where retrying the endStatus session would be necessary?
If that approach is taken, does it still fail to resolve the issue mentioned?

TimeoutRollbacked and Rollbacked residues #6875

Lastly, was this PR ultimately decided to simply retry the end status session, or was there another approach considered?

I completely agree that failed transactions should not be retried but should instead be handled by the operations team. However, I’m curious about how the issue is addressed, so I wanted to ask.

funky-eyes · 2025-02-18T02:37:45Z

For the reasons I mentioned earlier:

IO exceptions, such as disk or network issues
Process crashes (whether it's the seata-server or the database, etc.)

In the above points or some examples I did not mention, transactions are not deleted in the end state, leaving residual data.

…sion.java

YongGoose

@funky-eyes

Please take a look :)
If everything looks good, please proceed with committing the suggestion.

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java

server/src/main/java/org/apache/seata/server/session/SessionHelper.java

server/src/main/java/org/apache/seata/server/session/GlobalSession.java

…ltCoordinator.java Co-authored-by: Yongjun Hong <[email protected]>

…sion.java Co-authored-by: Yongjun Hong <[email protected]>

…lper.java Co-authored-by: Yongjun Hong <[email protected]>

YongGoose · 2025-02-18T04:41:51Z

@funky-eyes

Is there anything else left to do?

funky-eyes

LGTM

Fix: add compensation handling for TimeoutRollbacked and Rollbacked s…

2bcff1e

…tates

YongGoose mentioned this pull request Jan 31, 2025

TimeoutRollbacked and Rollbacked residues #6875

Closed

1 task

funky-eyes reviewed Feb 5, 2025

View reviewed changes

Merge branch '2.x' into feature/6875

54ee02d

funky-eyes added first-time contributor first-time contributor module/server server module and removed first-time contributor first-time contributor labels Feb 5, 2025

Merge branch '2.x' into feature/6875

4e21ee1

Merge branch '2.x' into feature/6875

61177a7

YongGoose mentioned this pull request Feb 11, 2025

Refactor: Refactoring the Result Class #7158

Open

YongGoose added 2 commits February 12, 2025 19:08

Add compensation handling for TimeoutRollbacked and Rollbacked states

fc28796

Merge branch '2.x' into feature/6875

031776d

YongGoose requested a review from funky-eyes February 12, 2025 10:09

YongGoose changed the title ~~Add compensation handling for TimeoutRollbacked and Rollbacked states~~ feature: Implement Scheduled Handling for Rollbacked and TimeoutRollbacked Transaction Feb 12, 2025

Add changes

1978ae0

funky-eyes reviewed Feb 13, 2025

View reviewed changes

YongGoose requested a review from funky-eyes February 13, 2025 06:07

YongGoose added 2 commits February 15, 2025 00:00

Add end status

a9ec30f

Create a corresponding RETRY_DEAD_THRESHOLD for the end state

997775c

funky-eyes reviewed Feb 15, 2025

View reviewed changes

YongGoose and others added 3 commits February 15, 2025 19:02

Update server/src/main/java/org/apache/seata/server/session/GlobalSes…

63b3c04

…sion.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

b44aa40

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

bea1ee1

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

YongGoose changed the title ~~feature: Implement Scheduled Handling for Rollbacked and TimeoutRollbacked Transaction~~ feature: Implement Scheduled Handling for end status Transaction Feb 15, 2025

rename configuration items and supplement

932c51f

update changes

194bc43

funky-eyes reviewed Feb 16, 2025

View reviewed changes

changes/zh-cn/2.x.md Outdated Show resolved Hide resolved

common/src/main/java/org/apache/seata/common/DefaultValues.java Outdated Show resolved Hide resolved

YongGoose and others added 2 commits February 16, 2025 16:08

Update changes/zh-cn/2.x.md

074d601

Co-authored-by: funkye <[email protected]>

Update common/src/main/java/org/apache/seata/common/DefaultValues.java

c48f117

Co-authored-by: funkye <[email protected]>

YongGoose requested a review from funky-eyes February 16, 2025 07:09

funky-eyes reviewed Feb 17, 2025

View reviewed changes

YongGoose and others added 5 commits February 17, 2025 11:12

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

bb2e13c

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

f3a34fb

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

13a0436

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

Managing transaction end states in SessionHelper

5837ae6

Rename rollbacked to status

113a7fd

funky-eyes reviewed Feb 18, 2025

View reviewed changes

YongGoose and others added 4 commits February 18, 2025 10:26

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

b5fe01b

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/session/SessionHe…

4f6a7f5

…lper.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

5996667

…ltCoordinator.java Co-authored-by: funkye <[email protected]>

Update server/src/main/java/org/apache/seata/server/session/SessionHe…

4c031cd

…lper.java Co-authored-by: funkye <[email protected]>

YongGoose commented Feb 18, 2025

View reviewed changes

server/src/main/java/org/apache/seata/server/session/GlobalSession.java Outdated Show resolved Hide resolved

YongGoose requested a review from funky-eyes February 18, 2025 01:54

Update server/src/main/java/org/apache/seata/server/session/GlobalSes…

6d85184

…sion.java

YongGoose commented Feb 18, 2025

View reviewed changes

server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java Outdated Show resolved Hide resolved

server/src/main/java/org/apache/seata/server/session/SessionHelper.java Outdated Show resolved Hide resolved

YongGoose commented Feb 18, 2025

View reviewed changes

server/src/main/java/org/apache/seata/server/session/GlobalSession.java Outdated Show resolved Hide resolved

funky-eyes and others added 3 commits February 18, 2025 11:07

Update server/src/main/java/org/apache/seata/server/coordinator/Defau…

4e48e40

…ltCoordinator.java Co-authored-by: Yongjun Hong <[email protected]>

Update server/src/main/java/org/apache/seata/server/session/GlobalSes…

6c99286

…sion.java Co-authored-by: Yongjun Hong <[email protected]>

Update server/src/main/java/org/apache/seata/server/session/SessionHe…

bd9f7dd

…lper.java Co-authored-by: Yongjun Hong <[email protected]>

funky-eyes approved these changes Feb 18, 2025

View reviewed changes

funky-eyes merged commit 46c9b63 into apache:2.x Feb 18, 2025
8 checks passed

feature: Implement scheduled handling for end status transaction #7133

feature: Implement scheduled handling for end status transaction #7133

Conversation

YongGoose commented Jan 31, 2025 • edited Loading

Ⅰ. Describe what this PR did

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

YongGoose commented Feb 5, 2025

funky-eyes commented Feb 7, 2025

YongGoose commented Feb 7, 2025

funky-eyes commented Feb 7, 2025

codecov bot commented Feb 8, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose commented Feb 17, 2025

funky-eyes left a comment

Choose a reason for hiding this comment

YongGoose commented Feb 18, 2025 • edited Loading

funky-eyes commented Feb 18, 2025 • edited Loading

YongGoose left a comment

Choose a reason for hiding this comment

YongGoose commented Feb 18, 2025

funky-eyes left a comment

Choose a reason for hiding this comment

YongGoose commented Jan 31, 2025 •

edited

Loading

YongGoose Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 8, 2025 •

edited

Loading

YongGoose Feb 14, 2025 •

edited

Loading

YongGoose Feb 14, 2025 •

edited

Loading

YongGoose Feb 15, 2025 •

edited

Loading

YongGoose commented Feb 18, 2025 •

edited

Loading

funky-eyes commented Feb 18, 2025 •

edited

Loading