feat(client): add GcsBidiWriteChannel for bidi appendable uploads - #366
feat(client): add GcsBidiWriteChannel for bidi appendable uploads#366animesh-g wants to merge 12 commits into
Conversation
…GcsBidiWriteChannel
- Add checkNotNull preconditions on constructors and write(ByteBuffer) in GcsBidiWriteChannel - Use double-checked locking in close() to avoid holding monitor lock unnecessarily - Add debug logging on successful close of GcsBidiWriteChannel - Update GcsBidiWriteChannelTest method names to remove redundant 'test' prefix - Add unit tests for null preconditions in GcsBidiWriteChannelTest
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds support for bidirectional (appendable) writes to Google Cloud Storage. By leveraging the GCS client library's BlobAppendableUpload, this feature allows for more flexible, incremental data uploads. The changes include the implementation of a new write channel, updates to configuration handling, and integration into the existing GcsClient, along with comprehensive unit tests to verify the new functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces GcsBidiWriteChannel to support bidirectional and appendable uploads to Google Cloud Storage, along with corresponding configuration options in GcsWriteOptions and integration in GcsClientImpl. The review feedback focuses on simplifying GcsBidiWriteChannel by removing the redundant bidiBytesWritten field and its associated overrides, and refactoring the close() method to ensure the channel is correctly marked as closed even when exceptions occur. Additionally, the reviewer recommends adding @NonNull annotations to constructor parameters to align with the style guide, passing resolvedWriteOptions to createBlobInfo in GcsClientImpl to prevent potential null pointer issues, and updating the unit tests to reflect these improvements.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
============================================
- Coverage 98.24% 98.18% -0.06%
- Complexity 597 611 +14
============================================
Files 41 42 +1
Lines 1876 1926 +50
Branches 180 188 +8
============================================
+ Hits 1843 1891 +48
Misses 15 15
- Partials 18 20 +2
🚀 New features to boost your workflow:
|
…on, and close pattern
…e private fields in GcsWriteChannel
…Channel and GcsBidiWriteChannel
animesh-g
changed the title
…s to GcsBidiWriteChannelTest
animesh-g
changed the title
| return; | ||
| } | ||
|
|
||
| synchronized (this) { |
There was a problem hiding this comment.
You can synchronize the method itself on place of synchronized(this). This will reduce the nesting.
If intention was to do double checking, then we need to check if(closed) inside synchronized block as well else two concurrent thread will evaluate outer condition to false and wait on getting the lock to enter critical section and will execute all the code. However, given terminal method close, I think it is ok to synchronize the whole method itself in favour of code complexity, there is negligible performance difference.
| try { | ||
| gcsAppendChannel.close(); | ||
| } catch (StorageException | IOException e) { | ||
| throw handleException(e, "close"); |
There was a problem hiding this comment.
Is it intentional to drop the parent exceptions, ex: exception of super.close() ?
| clientOptions.getGcsReadOptions().isBidiReadEnabled() | ||
| ? StorageOptions.grpc() | ||
| : StorageOptions.newBuilder(); | ||
| || clientOptions.getGcsWriteOptions().isBidiWriteEnabled(); |
There was a problem hiding this comment.
Is there a scenario where customers set these flags seperately ? If not, why not simplify it as clientOptions.isBidiEnabled() on place of separate flag for read and write ?
2 participants
Type of Change
feat: A new featurefix: A bug fixdocs: Documentation only changesstyle: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)refactor: A code change that neither fixes a bug nor adds a featureperf: A code change that improves performancetest: Adding missing tests or correcting existing testschore: Changes to the build process or auxiliary tools and libraries such as documentation generationDescription
What?
Introduces GcsBidiWriteChannel to support bidirectional, appendable streaming uploads to Google Cloud Storage via the gRPC BlobAppendableUpload session API (Storage.BidiWriteObject).
Why?
This enables low-latency streaming writes and optional object finalization on close.
Checklist
feat(core): ...)Generated/Assisted by Agent? [Yes]