Skip to content

feat(storage): implement 2 MiB coalescing buffer for appendable upload - #6571

Open
vsharonlynn wants to merge 1 commit into
googleapis:mainfrom
vsharonlynn:buffer
Open

feat(storage): implement 2 MiB coalescing buffer for appendable upload#6571
vsharonlynn wants to merge 1 commit into
googleapis:mainfrom
vsharonlynn:buffer

Conversation

@vsharonlynn

Copy link
Copy Markdown
Contributor

No description provided.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Aug 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 introduces a CoalescingBuffer to batch client append operations into standard 2 MiB chunks, matching the GCS protocol limit, and integrates it into the AppendableObjectWriterTransport along with updated unit tests. The review feedback suggests optimizing the buffer's ingestion logic by using direct slicing to avoid atomic reference counting overhead, and refactoring the flush method to remove an unnecessary else block in compliance with the repository style guide.

Comment on lines +59 to +60
let prefix = chunk.slice(..needed);
self.buffer.extend_from_slice(&prefix);

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.

medium

Slicing Bytes via chunk.slice(..needed) creates a new Bytes handle, which incurs atomic reference counting overhead. Since extend_from_slice only requires a byte slice (&[u8]), we can pass &chunk[..needed] directly to avoid this overhead. Please document this safety guarantee with a comment.

Suggested change
let prefix = chunk.slice(..needed);
self.buffer.extend_from_slice(&prefix);
// SAFETY: `needed` is guaranteed to be within the bounds of `chunk`.
self.buffer.extend_from_slice(&chunk[..needed]);
References
  1. When slicing a collection directly in Rust, document the safety guarantee with a comment.

Comment on lines +82 to +88
if self.buffer.is_empty() {
None
} else {
let residual = self.buffer.split().freeze();
self.buffer.reserve(COALESCING_CHUNK_SIZE);
Some(residual)
}

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.

medium

According to the repository style guide, we should avoid unnecessary else blocks to keep the main logic flow linear and reduce indentation.

        if self.buffer.is_empty() {
            return None;
        }
        let residual = self.buffer.split().freeze();
        self.buffer.reserve(COALESCING_CHUNK_SIZE);
        Some(residual)
References
  1. Avoid unnecessary else blocks to reduce indentation and keep the main logic flow linear. (link)

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.80838% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.51%. Comparing base (178dc27) to head (c133b14).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...torage/src/storage/bidi_write/coalescing_buffer.rs 95.86% 5 Missing ⚠️
src/storage/src/storage/bidi_write/transport.rs 95.65% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6571      +/-   ##
==========================================
- Coverage   96.52%   96.51%   -0.01%     
==========================================
  Files         304      305       +1     
  Lines       87969    88123     +154     
==========================================
+ Hits        84908    85054     +146     
- Misses       3061     3069       +8     

☔ 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.

@vsharonlynn
vsharonlynn marked this pull request as ready for review August 28, 2026 08:57
@vsharonlynn
vsharonlynn requested review from a team as code owners August 28, 2026 08:57
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.

1 participant