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.

Sign up for GitHub

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

Jump to bottom

feat(datastore): Adding BeginLater and transaction state #8984

Merged
merged 5 commits into from
Feb 5, 2024
Merged

feat(datastore): Adding BeginLater and transaction state #8984

merged 5 commits into from
Feb 5, 2024
+43 −12

Conversation

bhshkh
Copy link
Contributor

@bhshkh bhshkh commented Nov 7, 2023 •
edited
Loading

This PR adds following variables:

  • BeginLater transaction option
  • State enum in the transaction struct to track state of the transaction. Before this PR, transaction id was being used to track state of the transaction.
  • Currently, when a user instantiates a transaction object, the transaction options passed in the constructor (NewTransaction method) are being used only in the constructor itself to make a BeginTransaction rpc call and obtain transaction id from its response. So, there was no need to store these options.
    Now, these transaction options need to be saved to be used in BeginLater transactions in future PRs. So, a new field "settings" in the transaction struct has been added in this PR.

@bhshkh bhshkh requested review from a team as code owners November 7, 2023 22:31
@bhshkh bhshkh requested a review from enocom November 7, 2023 22:31
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the Datastore API. labels Nov 7, 2023
@bhshkh bhshkh marked this pull request as draft November 7, 2023 22:34
@bhshkh bhshkh marked this pull request as ready for review November 7, 2023 23:11
@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Dec 8, 2023
gkevinzheng
gkevinzheng reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@gkevinzheng gkevinzheng left a comment

Choose a reason for hiding this comment

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

LGTM

@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Jan 7, 2024
cindy-peng
cindy-peng reviewed Feb 5, 2024
View reviewed changes
datastore/transaction.go Outdated
// If there are no read operations on transaction, BeginTransaction RPC call is made
// before rollback or commit
// Currently, this setting is set but unused
// TODO: Use this setting
Copy link
Contributor

@cindy-peng cindy-peng Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Do we usually add buganizer ids for TODOs? go/todo-style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@bhshkh bhshkh requested a review from a team as a code owner February 5, 2024 18:14
@bhshkh bhshkh enabled auto-merge (squash) February 5, 2024 19:08
@bhshkh bhshkh merged commit 5f8e21f into googleapis:main Feb 5, 2024
9 checks passed
@release-please release-please bot mentioned this pull request Feb 5, 2024
Merged
@bhshkh bhshkh deleted the feature/new-transaction-1 branch February 5, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkevinzheng gkevinzheng gkevinzheng left review comments

@cindy-peng cindy-peng cindy-peng approved these changes

@enocom enocom Awaiting requested review from enocom

Assignees
No one assigned
Labels
api: datastore Issues related to the Datastore API. size: m Pull request size is medium. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bhshkh @gkevinzheng @cindy-peng