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 inconsistent transaction process if exception thrown #56153

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

starrocks-xupeng
Copy link
Contributor

@starrocks-xupeng starrocks-xupeng commented Feb 21, 2025

Why I'm doing:

What I'm doing:

  1. modify transaction state in memory
  2. call afterStateTransform
  3. persist transaction log

if step 1 succeeds, step 2 must not throw exception, avoid causing inconsistency between memory info(step 1) and disk info(step 3)

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
transactionState.afterStateTransform(TransactionStatus.VISIBLE, txnOperated, "");
} catch (Throwable t) {
LOG.warn("transaction after state transform failed: {}", transactionState, t);
}
}
persistTxnStateInTxnLevelLock(transactionState);

Copy link

Choose a reason for hiding this comment

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

The most risky bug in this code is:
Returning null when txnOperated is false may not be handled properly by the caller, leading to potential null pointer exceptions or logical errors downstream.

You can modify the code like this:

// Check for txnOperated before returning null
if (!txnOperated) {
    LOG.error("Transaction operation failed. Returning default value or taking corrective action.");
    // Handle the null return case more robustly here, e.g., return an empty VisibleStateWaiter.
    return new VisibleStateWaiter(); // Example, adjust to actual required behavior
}

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
19.5% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

Copy link

[FE Incremental Coverage Report]

fail : 16 / 38 (42.11%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/lake/compaction/CompactionScheduler.java 0 8 00.00% [136, 137, 138, 141, 142, 414, 415, 416]
🔵 com/starrocks/transaction/GlobalTransactionMgr.java 1 3 33.33% [345, 346]
🔵 com/starrocks/transaction/DatabaseTransactionMgr.java 15 27 55.56% [429, 430, 526, 527, 531, 617, 618, 1289, 1290, 1324, 1994, 1995]

Copy link

[BE Incremental Coverage Report]

pass : 0 / 0 (0%)

@kevincai kevincai changed the title [BugFix] fix inconsistent transaction process if expcetion thrown [BugFix] fix inconsistent transaction process if exception thrown Feb 23, 2025
@wyb wyb enabled auto-merge (squash) February 25, 2025 02:13
@andyziye andyziye disabled auto-merge February 25, 2025 05:31
@andyziye andyziye merged commit 59aaa47 into StarRocks:main Feb 25, 2025
130 of 136 checks passed
Copy link

@Mergifyio backport branch-3.4

@github-actions github-actions bot removed the 3.4 label Feb 25, 2025
Copy link

@Mergifyio backport branch-3.3

@github-actions github-actions bot removed the 3.3 label Feb 25, 2025
Copy link
Contributor

mergify bot commented Feb 25, 2025

backport branch-3.4

✅ Backports have been created

Copy link
Contributor

mergify bot commented Feb 25, 2025

backport branch-3.3

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 25, 2025
…6153)

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
(cherry picked from commit 59aaa47)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/transaction/DatabaseTransactionMgr.java
mergify bot pushed a commit that referenced this pull request Feb 25, 2025
…6153)

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
(cherry picked from commit 59aaa47)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/transaction/DatabaseTransactionMgr.java
starrocks-xupeng added a commit to starrocks-xupeng/starrocks that referenced this pull request Feb 25, 2025
…ckport StarRocks#56153)

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
starrocks-xupeng added a commit to starrocks-xupeng/starrocks that referenced this pull request Feb 25, 2025
…ckport StarRocks#56153)

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
@starrocks-xupeng
Copy link
Contributor Author

#56242
3.3

@starrocks-xupeng
Copy link
Contributor Author

#56243
3.4

kevincai pushed a commit that referenced this pull request Feb 25, 2025
…ckport #56153) (#56243)

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
kevincai pushed a commit that referenced this pull request Feb 26, 2025
…ckport #56153) (#56242)

Signed-off-by: starrocks-xupeng <xupeng@starrocks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants