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

Make data.Transaction.Rollback a noop if the transaction was already committed #3104

Merged
merged 4 commits into from
Sep 6, 2022

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Sep 1, 2022

Summary

Branched from #3103

This PR does some cleanup around transactions. Best viewed by individual commit.

  1. make data.Transaction.Rollback a noop if the transaction was already committed. This makes it possible to defer the rollback, and simplifies the logic where we use transactions.
  2. remove withDBTxn since it is no longer necessary with the above change.
  3. extracts a few small things from wrapRoute. This function has grown a lot, moving a few small things out of it helps keep it focused and easy to read.

return err
}

trimWhitespace(req)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moved into readRequest since they are both related to reading the request into the struct.

Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

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

elegant solution 👍

internal/server/data/data.go Outdated Show resolved Hide resolved
and move trimWhitespace into readRequest since it's related to reading the request.
…itted

This allows us to remove withDBTxn, and also to simplify the logic in wrapRoute.
Co-authored-by: Bruce MacDonald <brucewmacdonald@gmail.com>
Base automatically changed from dnephin/fix-request-txn to main September 6, 2022 16:29
@dnephin dnephin merged commit b298cd2 into main Sep 6, 2022
@dnephin dnephin deleted the dnephin/reduce-wrap-route branch September 6, 2022 16:34
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.

2 participants