feat: Add Bidi write feature - #2343
Conversation
product-auto-label
Bot
added
size: xl
| * {@code 262144 (256 KiB)} | ||
| * @return The new instance | ||
| * @see #getBufferSize() | ||
| * @since 2.26.0 This new api is in preview and is subject to breaking changes. |
There was a problem hiding this comment.
I'm assuming this version needs to be updated to whichever version is used at time of release? (same goes for other occurences).
| @@ -0,0 +1,180 @@ | |||
| /* | |||
| * Copyright 2023 Google LLC | |||
| } | ||
| } | ||
|
|
||
| static class BidiObserver implements ApiStreamObserver<BidiWriteObjectResponse> { |
There was a problem hiding this comment.
Could you add comments on what this Observer does to support Bidi?
IIUC, this is how you close the stream.
| } | ||
|
|
||
| if (message.getWriteOffset() > 0 && !message.getFinishWrite()) { | ||
| b.clearObjectChecksums(); |
There was a problem hiding this comment.
Does this remove checksums for every message after initial / ignoring last? I would expect checksums to be supplied for intermediate messages as well.
There was a problem hiding this comment.
Ignore my understanding was incomplete.
| return b.build(); | ||
| } | ||
|
|
||
| private static BidiWriteObjectRequest possiblyPairDownBidiRequest( |
There was a problem hiding this comment.
Please add unit tests for this helper.
There was a problem hiding this comment.
My original intention was to introduce @VisibleForTesting for the specific comment thread around removing checksum in intermediate messages which is a misunderstanding on my part after talking with Jesse.
States that checksum can only be provided in first or last message sent to API so this comment and the one below are obsolete.
| } | ||
| BidiWriteObjectRequest message = | ||
| BidiWriteObjectRequest.newBuilder().setFlush(true).setStateLookup(true).build(); | ||
| stream.onNext(message); |
There was a problem hiding this comment.
IIUC the main difference between bidi and non-bidi is that stream.onComplete() is not called until the Observer observes onCompleted?
There was a problem hiding this comment.
Correct, current flow calls onComplete on each flush. Bidi will only call onComplete once, at the end of the upload
frankyn
left a comment
frankyn
left a comment
There was a problem hiding this comment.
Remaining comments are nits; PR LGTM, thanks for the offline discussion @JesseLovelace
3 participants
This adds a new BlobWriteSessionConfig which uses the Bidi streaming feature. It's largely the same as our normal resumable upload flow, with two key differences:
-Instead of closing and re-opening a stream on each flush, we keep a stream open for the life of the upload
-On each flush, it sends a signal to GCS with the "flush" and "state_lookup" flags (which are exclusive to bidi writes) set to true
I still need to run a formal profiling test, but basic profiling in IntelliJ is consistent with the 10-15% increase we're expecting