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

kv,sql: simplify the Txn API by removing 2 cleanup functions #88101

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

lidorcarmel
Copy link
Contributor

@lidorcarmel lidorcarmel commented Sep 18, 2022

Txn.CleanupOnError() basically does a rollback, and in addition takes an error only for the purpose of logging it.

Txn.CommitOrCleanup() tries to commit and if unsuccessful it tries a rollback. The error from the rollback is logged but not returned, the error from the commit is returned.

Removing these 2 functions means that the caller should call Commit and Rollback directly when needed, and handle the returned errors. For example, sql may need to log errors to a different channel from the one used but Txn, and tests may want to fail when a Rollback fails unexpectedly. This PR removes those functions.

Release note: None
Epic: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lidorcarmel lidorcarmel marked this pull request as ready for review September 18, 2022 15:46
@lidorcarmel lidorcarmel requested review from a team as code owners September 18, 2022 15:46
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm:, but let's hold off on merging this while in stability.

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

Txn.CleanupOnError() basically does a rollback, and in addition takes an error
only for the purpose of logging it.

Txn.CommitOrCleanup() tries to commit and if unsuccessful it tries a rollback.
The error from the rollback is logged but not returned, the error from the
commit is returned.

Removing these 2 functions means that the caller should call Commit and
Rollback directly when needed, and handle the returned errors. For example,
sql may need to log errors to a different channel from the one used but Txn,
and tests may want to fail when a Rollback fails unexpectedly. This PR
removes those functions.

Release note: None
@lidorcarmel
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 1, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Nov 1, 2022

Build succeeded:

@craig craig bot merged commit 14249a5 into cockroachdb:master Nov 1, 2022
@lidorcarmel lidorcarmel deleted the lidor_drop_cleanup_on_error branch January 24, 2023 20:07
@lidorcarmel lidorcarmel restored the lidor_drop_cleanup_on_error branch January 24, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants