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

chore(storage): implement Read with gRPC #4401

Merged
merged 97 commits into from Aug 20, 2021
Merged

chore(storage): implement Read with gRPC #4401

merged 97 commits into from Aug 20, 2021

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Jul 8, 2021 •
edited

Adds (unexported) first pass implementation of gRPC-based Reader with integration tests based on the Storage v2 API.

This includes:

  • Stream reopening/retry
  • CRC32 checksum verification
  • Integration tests
  • local caching of left over response data

Note: This would normally be a feat but we don't want to trigger a semver change.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 8, 2021
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jul 8, 2021
@noahdietz noahdietz marked this pull request as ready for review July 22, 2021 17:58
@noahdietz noahdietz requested a review from a team July 22, 2021 17:58
@noahdietz noahdietz requested a review from a team as a code owner July 22, 2021 17:58
@noahdietz noahdietz changed the title feat(storage): implement Read with gRPC chore(storage): implement Read with gRPC Jul 22, 2021
@noahdietz
Copy link
Contributor Author

cc: @danielduhh as an fyi

@noahdietz noahdietz requested a review from tritone August 13, 2021 21:52
tritone
tritone reviewed Aug 19, 2021
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Couple additional comments on testing. This is looking really good aside from my other few open comments!

storage/integration_test.go Outdated Show resolved Hide resolved
storage/integration_test.go Outdated Show resolved Hide resolved
storage/integration_test.go Outdated Show resolved Hide resolved
@noahdietz noahdietz requested a review from tritone August 19, 2021 20:50
tritone
tritone approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

After a bunch of wrangling, this LGTM. Thanks for all your work on this @noahdietz !!

@coryan @codyoss @broady feel free to take another look; if we don't hear from you we'll merge tomorrow afternoon.

@noahdietz noahdietz requested a review from codyoss August 19, 2021 23:20
@tritone tritone added the automerge Merge the pull request once unit tests and other checks pass. label Aug 20, 2021
@tritone tritone merged commit 68b8eb8 into googleapis:master Aug 20, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 20, 2021
@noahdietz noahdietz deleted the storage-grpc branch August 20, 2021 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coryan coryan coryan left review comments

@tritone tritone tritone approved these changes

@codyoss codyoss Awaiting requested review from codyoss

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@noahdietz @codyoss @coryan @tritone @kokoro-team