optimize: raft mode performs transaction size check in advance #7344

funky-eyes · 2025-05-18T06:04:08Z

I have registered the PR changes.

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

codecov · 2025-05-18T06:21:48Z

Codecov Report

Attention: Patch coverage is 57.89474% with 32 lines in your changes missing coverage. Please review.

Project coverage is 54.92%. Comparing base (766be77) to head (cc0ffe3).
Report is 1 commits behind head on 2.x.

Files with missing lines	Patch %	Lines
...erver/storage/raft/session/RaftSessionManager.java	0.00%	14 Missing ⚠️
...org/apache/seata/server/session/GlobalSession.java	67.74%	6 Missing and 4 partials ⚠️
...org/apache/seata/server/session/BranchSession.java	74.19%	2 Missing and 6 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #7344      +/-   ##
============================================
+ Coverage     54.90%   54.92%   +0.02%     
- Complexity     7399     7415      +16     
============================================
  Files          1181     1181              
  Lines         42106    42136      +30     
  Branches       4935     4945      +10     
============================================
+ Hits          23119    23144      +25     
+ Misses        16818    16813       -5     
- Partials       2169     2179      +10

Files with missing lines	Coverage Δ
...org/apache/seata/server/session/BranchSession.java	`86.79% <74.19%> (+2.54%)`	⬆️
...org/apache/seata/server/session/GlobalSession.java	`69.49% <67.74%> (+1.16%)`	⬆️
...erver/storage/raft/session/RaftSessionManager.java	`8.26% <0.00%> (-0.22%)`	⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull Request Overview

This PR adds pre-emptive size checks for global and branch sessions in Raft mode to prevent oversized transactions, and updates related tests and session manager calls.

Introduces checkSize methods in GlobalSession and BranchSession to validate payload sizes.
Calls checkSize in RaftSessionManager before Raft operations (onBegin, onAddBranch, etc.).
Adds unit tests verifying that sessions exceeding size limits throw TransactionException.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
server/src/test/java/org/apache/seata/server/session/GlobalSessionTest.java	Added `checkSizeTest` for `GlobalSession`.
server/src/test/java/org/apache/seata/server/session/BranchSessionTest.java	Added `checkSizeTest` for `BranchSession` and updated `codecTest` signature.
server/src/main/java/org/apache/seata/server/storage/raft/session/RaftSessionManager.java	Imported and invoked `checkSize`, refactored exception construction.
server/src/main/java/org/apache/seata/server/session/GlobalSession.java	Refactored `encode` to use `checkSize`, implemented new `checkSize` and helper methods.
server/src/main/java/org/apache/seata/server/session/BranchSession.java	Refactored `encode` to use `checkSize`, implemented new `checkSize` and helper methods.
changes/zh-cn/2.x.md	Updated changelog entry for size-check feature.
changes/en-us/2.x.md	Updated changelog entry for size-check feature.

Comments suppressed due to low confidence (2)

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

[nitpick] Method name 'calGlobalSessionSize' is abbreviated; consider renaming to 'calculateGlobalSessionSize' for better readability.

private int calGlobalSessionSize(byte[] byApplicationIdBytes, byte[] byServiceGroupBytes, byte[] byTxNameBytes, byte[] xidBytes, byte[] applicationDataBytes) {

server/src/main/java/org/apache/seata/server/session/BranchSession.java:449

[nitpick] Method name 'calBranchSessionSize' could be more descriptive as 'calculateBranchSessionSize'.

private int calBranchSessionSize(byte[] resourceIdBytes, byte[] lockKeyBytes, byte[] clientIdBytes, byte[] applicationDataBytes, byte[] xidBytes) {

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

server/src/main/java/org/apache/seata/server/storage/raft/session/RaftSessionManager.java

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

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

…ion/RaftSessionManager.java Co-authored-by: Copilot <[email protected]>

YongGoose

Left few comments!
Please take a look :)

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

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

server/src/test/java/org/apache/seata/server/session/GlobalSessionTest.java

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

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

…sion.java

YongGoose · 2025-05-19T13:41:48Z

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

+        if (!RaftServerManager.isRaftMode()) {
+            checkSize(resourceIdBytes, lockKeyBytes, clientIdBytes, applicationDataBytes, xidBytes);


I have one question.

The title of the PR says that the size check for transactions is performed in advance in Raft mode, but the relevant code seems to perform the check in advance even in modes other than Raft.

If I’ve misunderstood something, I apologize in advance :)

YongGoose

I’ve written one question above!

Also, I think it would be great to add a unit test to verify that the checkSize method is called when in Raft mode.

If that unit test is added, I believe a full integration test may not be strictly necessary.

funky-eyes · 2025-05-19T14:03:23Z

I’ve written one question above!

Also, I think it would be great to add a unit test to verify that the checkSize method is called when in Raft mode.

If that unit test is added, I believe a full integration test may not be strictly necessary.

In "file" mode, because GlobalSession and BranchSession instances need to be written to disk, their encode method is inevitably called when these instances are added. At this stage, the transaction size is validated. Conversely, "db" and "redis" modes do not require writing transaction information to disk, so the encode method is not called at all; consequently, these modes do not perform a transaction size check.

In "raft" mode, however, transaction information is stored entirely in memory. It's only when a Raft snapshot needs to be created that these in-memory transactions are flushed to disk. If the encode method is executed at this point, it's too late. This can lead to snapshot creation failure because if a transaction in memory exceeds the maximum size limit, a runtime exception is thrown, causing the snapshot process to fail. Therefore, for "raft" mode, the transaction size check is performed belatedly. The purpose of this pull request (PR) is to implement this check at an earlier stage to prevent the aforementioned issue.

funky-eyes · 2025-05-19T14:20:14Z

I’ve written one question above!

Also, I think it would be great to add a unit test to verify that the checkSize method is called when in Raft mode.

If that unit test is added, I believe a full integration test may not be strictly necessary.

It's not possible to construct multiple Raft nodes to form a Raft cluster within unit tests. Therefore, I believe that calling the checkSize method in RaftSessionManager is not fundamentally different from directly calling the checkSize method of GlobalSession and BranchSession, unless we use integration tests instead of unit tests.

YongGoose · 2025-05-19T14:26:26Z

I’ve written one question above!
Also, I think it would be great to add a unit test to verify that the checkSize method is called when in Raft mode.
If that unit test is added, I believe a full integration test may not be strictly necessary.

In "file" mode, because GlobalSession and BranchSession instances need to be written to disk, their encode method is inevitably called when these instances are added. At this stage, the transaction size is validated. Conversely, "db" and "redis" modes do not require writing transaction information to disk, so the encode method is not called at all; consequently, these modes do not perform a transaction size check.

In "raft" mode, however, transaction information is stored entirely in memory. It's only when a Raft snapshot needs to be created that these in-memory transactions are flushed to disk. If the encode method is executed at this point, it's too late. This can lead to snapshot creation failure because if a transaction in memory exceeds the maximum size limit, a runtime exception is thrown, causing the snapshot process to fail. Therefore, for "raft" mode, the transaction size check is performed belatedly. The purpose of this pull request (PR) is to implement this check at an earlier stage to prevent the aforementioned issue.

Oh, I see !!
Thank you for the detailed explanation. 🙇🏻

YongGoose

LGTM 👍🏻
Integration tests can be added here or in a separate PR—both options are fine :)

YongGoose · 2025-05-19T15:16:35Z

LGTM 👍🏻 Integration tests can be added here or in a separate PR—both options are fine :)

I think it would be good to proceed with the integration test in the following way.

YongGoose@16e477e

While working locally to create a PR for your branch in order to help you, I got stuck on the part where the RaftServerManager is initialized. 😭

Have a good night!

funky-eyes · 2025-05-19T15:34:24Z

LGTM 👍🏻 Integration tests can be added here or in a separate PR—both options are fine :)

I think it would be good to proceed with the integration test in the following way.

YongGoose@16e477e

While working locally to create a PR for your branch in order to help you, I got stuck on the part where the RaftServerManager is initialized. 😭

Have a good night!

Thank you for your assistance. However, I believe that while the mock approach is meaningful for testing and verification in a single-instance deployment, it may not fully and accurately validate the process of adding transactions in Raft mode, which is a cluster-based setup.

…e#7344)

raft mode performs transaction size check in advance.

ea9e2eb

funky-eyes added this to the 2.5.0 milestone May 18, 2025

update

a5febc5

update

9070b1f

funky-eyes requested a review from Copilot May 18, 2025 08:40

Copilot AI reviewed May 18, 2025

View reviewed changes

funky-eyes and others added 4 commits May 18, 2025 21:37

update

375e5ef

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

e0224e6

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

Update server/src/main/java/org/apache/seata/server/storage/raft/sess…

3dd5e53

…ion/RaftSessionManager.java Co-authored-by: Copilot <[email protected]>

update

d7f0289

funky-eyes added module/server server module type: optimize store: raft labels May 19, 2025

funky-eyes requested review from YongGoose and slievrly May 19, 2025 02:28

YongGoose requested changes May 19, 2025

View reviewed changes

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

aabd866

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

funky-eyes commented May 19, 2025

View reviewed changes

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

funky-eyes added 3 commits May 19, 2025 17:37

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

425ce04

…sion.java

update

d8494f6

Merge branch '2.x' into 20250514

40135bf

funky-eyes requested a review from YongGoose May 19, 2025 13:05

YongGoose reviewed May 19, 2025

View reviewed changes

Merge branch '2.x' into 20250514

cc0ffe3

This comment was marked as spam.

Sign in to view

YongGoose approved these changes May 19, 2025

View reviewed changes

funky-eyes merged commit 3d6cf39 into apache:2.x May 19, 2025
9 checks passed

coderabbitai bot mentioned this pull request Jun 11, 2025

20250523 fork review YoWuwuuuw/incubator-seata#24

Closed

1 task

slievrly pushed a commit to slievrly/fescar that referenced this pull request Oct 21, 2025

optimize: raft mode performs transaction size check in advance (apach…

904a002

…e#7344)

YvCeung pushed a commit to YvCeung/incubator-seata that referenced this pull request Dec 25, 2025

optimize: raft mode performs transaction size check in advance (apach…

794d58f

…e#7344)

		if (!RaftServerManager.isRaftMode()) {
		checkSize(resourceIdBytes, lockKeyBytes, clientIdBytes, applicationDataBytes, xidBytes);

optimize: raft mode performs transaction size check in advance #7344

optimize: raft mode performs transaction size check in advance #7344

Uh oh!

Conversation

funky-eyes commented May 18, 2025

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

Uh oh!

codecov bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YongGoose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YongGoose May 19, 2025

Choose a reason for hiding this comment

Uh oh!

YongGoose left a comment

Choose a reason for hiding this comment

Uh oh!

funky-eyes commented May 19, 2025

Uh oh!

funky-eyes commented May 19, 2025

Uh oh!

YongGoose commented May 19, 2025

Uh oh!

This comment was marked as spam.

Uh oh!

YongGoose left a comment

Choose a reason for hiding this comment

Uh oh!

YongGoose commented May 19, 2025

Uh oh!

funky-eyes commented May 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented May 18, 2025 •

edited

Loading