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

feat(datastore): adds snapshot reads #6755

Merged
merged 35 commits into from
Oct 26, 2022
Merged

feat(datastore): adds snapshot reads #6755

merged 35 commits into from
Oct 26, 2022

Conversation

telpirion
Copy link
Contributor

@telpirion telpirion commented Sep 27, 2022

This PR adds snapshot reads to the client and transactions. Snapshot reads allow for a time-specific read-only version of the database to be accessed.

Note: This PR does not include integration tests, as read results do not indicate a read time.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the Datastore API. labels Sep 27, 2022
@telpirion telpirion marked this pull request as ready for review September 28, 2022 22:20
@telpirion telpirion requested review from enocom and a team as code owners September 28, 2022 22:20
@enocom enocom self-assigned this Sep 29, 2022
datastore/datastore.go Outdated Show resolved Hide resolved
datastore/datastore.go Outdated Show resolved Hide resolved
datastore/transaction.go Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Oct 10, 2022
datastore/datastore.go Outdated Show resolved Hide resolved
datastore/datastore_test.go Show resolved Hide resolved
datastore/datastore.go Show resolved Hide resolved
datastore/datastore.go Outdated Show resolved Hide resolved
}

ctx := context.Background()
client.WithReadOptions(ReadTime(time.Now()))
Copy link
Member

Choose a reason for hiding this comment

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

How are we checking that the read time is actually sent to the backend? Maybe I'm missing something, but I don't see how this test exercises that logic.

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 is just a unit test. Any check where we read the response's read time would be merely a result of how we set up the mock (and hence not useful as a test case).

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 test is really just to ensure that using Client.WithReadOptions() doesn't result in an error.

@telpirion telpirion requested a review from enocom October 12, 2022 19:45
datastore/datastore_test.go Outdated Show resolved Hide resolved
datastore/datastore_test.go Show resolved Hide resolved
@telpirion telpirion requested a review from enocom October 18, 2022 16:36
@telpirion telpirion enabled auto-merge (squash) October 26, 2022 18:44
@telpirion telpirion added the automerge Merge the pull request once unit tests and other checks pass. label Oct 26, 2022
@telpirion telpirion merged commit 9240741 into main Oct 26, 2022
@telpirion telpirion deleted the datastore-snapshot branch October 26, 2022 19:09
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants