-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Modify distributed transaction commit flow #16468
Conversation
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16468 +/- ##
==========================================
- Coverage 68.66% 68.63% -0.03%
==========================================
Files 1550 1552 +2
Lines 199307 199544 +237
==========================================
+ Hits 136846 136961 +115
- Misses 62461 62583 +122 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic LGTM
const DebugTwoPc = true | ||
|
||
// checkTestFailure is used to simulate failures in 2PC flow for testing when DebugTwoPc is true. | ||
func checkTestFailure(ctx context.Context, expectCaller string, target *querypb.Target) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put this in this file? and then a corresponding function in the other file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple reasons
- This is an unused function in the production build.
- Anyone can end up using it if this implementation remains in production code.
- Simple no-op production code will avoid any incidental calls to it.
Signed-off-by: Harshit Gangal <[email protected]>
return err | ||
} | ||
|
||
return txc.tabletGateway.ConcludeTransaction(ctx, mmShard.Target, dtid) | ||
// At this point, application can continue forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my own understanding - could we in theory do the clean up in a background thread and answer the user that their commit has been accepted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, yes, but it is better to call conclude now, than using goroutine or transaction resolver to conclude.
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Description
This PR updates the commit flow as per the new design doc on atomic distributed transactions.
show warnings
and then extracting transaction ID. Later can issueshow transaction status for '<id>'
to know the current state of it.Related Issue(s)
Checklist