Skip to content

feat(storage): implement API, request builders, integration tests#5957

Open
vsharonlynn wants to merge 12 commits into
googleapis:mainfrom
vsharonlynn:appendable_upload_4
Open

feat(storage): implement API, request builders, integration tests#5957
vsharonlynn wants to merge 12 commits into
googleapis:mainfrom
vsharonlynn:appendable_upload_4

Conversation

@vsharonlynn

@vsharonlynn vsharonlynn commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

This PR comes after PR #5956.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Jun 28, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements bidirectional streaming writes (Bidi Write) for Google Cloud Storage, introducing the AppendableObjectWriter along with request builders OpenAppendableObject and ReopenAppendableObject to support appending data to objects. It includes a background Worker to manage the gRPC stream, a Connector to handle connection handshakes and routing redirects, and comprehensive unit and integration tests. The review feedback highlights two opportunities in connector.rs to improve error handling by replacing .expect() calls with safe error propagation using ? when converting parameters to proto format.

Comment thread src/storage/src/storage/bidi_write/connector.rs Outdated
Comment thread src/storage/src/storage/bidi_write/connector.rs Outdated
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.81699% with 217 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.48%. Comparing base (5e91002) to head (5e066c1).

Files with missing lines Patch % Lines
src/storage/src/storage/open_appendable_object.rs 45.04% 111 Missing ⚠️
...rc/storage/src/storage/reopen_appendable_object.rs 68.64% 37 Missing ⚠️
src/storage/src/storage/bidi_write/worker.rs 82.03% 23 Missing ⚠️
src/storage/src/storage/stub.rs 0.00% 18 Missing ⚠️
src/storage/src/storage/bidi_write/transport.rs 96.03% 12 Missing ⚠️
src/storage/src/storage/bidi_write/connector.rs 98.03% 10 Missing ⚠️
src/storage/src/storage/bidi_write/redirect.rs 95.50% 4 Missing ⚠️
src/storage/src/storage/transport.rs 97.14% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5957      +/-   ##
==========================================
- Coverage   97.72%   97.48%   -0.24%     
==========================================
  Files         242      250       +8     
  Lines       60882    62351    +1469     
==========================================
+ Hits        59495    60782    +1287     
- Misses       1387     1569     +182     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tests/storage/src/bidi_write.rs Outdated
Comment thread tests/storage/src/bidi_write.rs Outdated
Comment thread tests/storage/src/bidi_write.rs Outdated
@vsharonlynn vsharonlynn force-pushed the appendable_upload_4 branch 6 times, most recently from e844df2 to 7e41183 Compare June 29, 2026 17:01
@vsharonlynn vsharonlynn force-pushed the appendable_upload_4 branch from 7e41183 to 5e066c1 Compare June 29, 2026 17:24
@vsharonlynn

Copy link
Copy Markdown
Contributor Author

I ran

$ cargo +nightly public-api -p google-cloud-storage diff origin/main..HEAD

The output is:

Removed items from the public API
=================================
(none)

Changed items in the public API
===============================
(none)

Added items to the public API
=============================
(none)

@vsharonlynn vsharonlynn marked this pull request as ready for review June 29, 2026 17:47
@vsharonlynn vsharonlynn requested review from a team as code owners June 29, 2026 17:47

@coryan coryan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is too large for a single PR, please split.

@vsharonlynn

vsharonlynn commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

This is too large for a single PR, please split.

Sorry about this. I have chained PRs, this has changes from #5956. Because my branches are in my fork, I can't diff this branch from the #5956 branch in a PR into this repository.

One way I found on how to view only the changes for this PR is to select the commits that don't belong to the previous PR:

image Happy to split it up if this still looks too large.

Another way is for us to wait until the previous PR is merged, then I'll rebase this before you take a look.

Please let me know if there is a more efficient way to do this...

Thank you.

@coryan

coryan commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Another way is for us to wait until the previous PR is merged, then I'll rebase this before you take a look.

Yes, that works for me.

Please let me know if there is a more efficient way to do this...

Support for stacked PRs (allegedly in the works at GitHub) may help, but I do not think that is available to us yet.

Normally we keep stacked PRs as draft. They are useful to give reviewers a sense of the overall change, but too large to review. Reviewing after a rebase is far more practical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants