chore: add BidiUploadState #3222
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
product-auto-label
bot
added
size: xl
BenWhitehead
force-pushed
the
nonblocking-appendable/05/consolidate-status-bin
branch
from
d79117e to
fc552a4
Compare
BenWhitehead
force-pushed
the
nonblocking-appendable/06/upload-state
branch
2 times, most recently
from
437797f to
af8b816
Compare
This will be used by an upcoming PR that will integrate it with a BidiWriteObject stream. At a high level this class hierarchy models the state and lifecycle events which happen when performing an upload -- in this change append specific, but many generalize to all uploads.
BenWhitehead
force-pushed
the
nonblocking-appendable/06/upload-state
branch
from
af8b816 to
75d4cfd
Compare
BrandonY
reviewed
Aug 11, 2025
| private static final Marker TRACE_EXIT = MarkerFactory.getMarker("exit"); | ||
|
|
||
| static final OneofDescriptor FIRST_MESSAGE_DESCRIPTOR = | ||
| BidiWriteObjectRequest.getDescriptor().getOneofs().stream() |
There was a problem hiding this comment.
This can't be the right way to get at the "first_message" field, can it?
There was a problem hiding this comment.
It's the easiest way to probe whether it is set or not. protoc doesn't generate a .hasFirstMessage() method like it does for optional fields. But the base message class has com.google.protobuf.GeneratedMessageV3#hasOneof(com.google.protobuf.Descriptors.OneofDescriptor) method. Using this to probe presence felt cleaner than m.hasUploadId() || m.hasWriteObjectSpec() || m.hasAppendObjectSpec().
| checkArgument( | ||
| !(first.hasChecksummedData() && second.hasChecksummedData()), | ||
| "attempting to merge two requests that both specify checksummed_data"); | ||
| BidiWriteObjectRequest.Builder b = first.toBuilder().mergeFrom(second); |
There was a problem hiding this comment.
Does this throw away the data from one of the two writes?
There was a problem hiding this comment.
The checkArgument on line 195-197 guarantees that at most one of the messages contains data. By the time we make it to mergeFrom, we know that only 0 or 1 of the messages contains any actual data. If there were a bug in the caller of this method, the exception from checkArgument would prevent it from silently making a bad mutation.
BrandonY
approved these changes
Aug 15, 2025
BenWhitehead
added a commit
that referenced
this pull request
Aug 20, 2025
## Description feat: *breaking behavior* rewrite Storage.blobAppendableUpload to be non-blocking and have improved throughput (#3231) Rewrite internals of BlobAppendableUpload to provide non-blocking write calls, and it take advantage of grpc async message handling. When `AppendableUploadWriteableByteChannel#write(ByteBuffer)` is called, an attempt will be made to enqueue the bytes in the outbound queue to GCS. If there is only enough room to partially consume the bytes provided in the `ByteBuffer` the write call will return early specifying the number of bytes actually consumed. As acknowledgements come in from gcs, enqueued messages will be evicted freeing space in the outbound queue. Thereby allowing more bytes to be consumed and enqueued. Given appendable objects are still in private preview I can't quote any metrics here, however preliminary benchmarking of several million objects across a range of sizes show across the board throughput improvments. Because the channel's write call is now non-blocking, if you want to block your application until the full buffer is consumed some new helper methods have been added in StorageChannelUtils to provide blocking behavior. A new method `MinFlushSizeFlushPolicy#withMaxPendingBytes(long)` has been added to allow limiting the number of pending outbound bytes. The default values is 16MiB, but can be configured lower if necessary. ## Release Notes BEGIN_COMMIT_OVERRIDE BEGIN_NESTED_COMMIT feat: *breaking behavior* rewrite Storage.blobAppendableUpload to be non-blocking and have improved throughput (#3231) END_NESTED_COMMIT BEGIN_NESTED_COMMIT feat: add StorageChannelUtils to provide helper methods to perform blocking read/write to/from non-blocking channels (#3231) END_NESTED_COMMIT BEGIN_NESTED_COMMIT feat: add MinFlushSizeFlushPolicy#withMaxPendingBytes(long) (#3231) END_NESTED_COMMIT BEGIN_NESTED_COMMIT fix: update BlobAppendableUploadConfig and FlushPolicy.MinFlushSizeFlushPolicy to default to 4MiB minFlushSize and 16MiB maxPendingBytes (#3249) END_NESTED_COMMIT BEGIN_NESTED_COMMIT fix: make FlushPolicy${Min,Max}FlushSizeFlushPolicy constructors private (#3217) END_NESTED_COMMIT END_COMMIMT_OVERRIDE ## Sub PRs This PR is made of up the following PRs, in sequence 1. #3217 2. #3218 3. #3219 4. #3220 5. #3221 6. #3222 7. #3223 8. #3224 9. #3225 10. #3226 11. #3227 12. #3228 13. #3229 14. #3230 15. #3235 16. #3236 17. #3241 18. #3242 19. #3246 20. #3248 21. #3249 22. #3252
|
Merged in #3231 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will be used by an upcoming PR that will integrate it with a BidiWriteObject stream.
At a high level this class hierarchy models the state and lifecycle events which happen when performing an upload -- in this change append specific, but many generalize to all uploads.