-
Notifications
You must be signed in to change notification settings - Fork 40
Backport to branch(3) : Add support for Cloud Storage #3187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @feeblefakie, 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 backports the functionality to integrate Google Cloud Storage as a new object storage option. It introduces new modules for Cloud Storage configuration and interaction, updates the build process to include necessary dependencies, and refines the core object storage framework to seamlessly incorporate this new backend. The changes ensure robust error handling and proper resource management for Cloud Storage operations, alongside updates to integration tests to validate the new functionality. Highlights
Ignored Files
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR adds support for Google Cloud Storage as an object storage backend. The changes are comprehensive, including configuration, implementation, and integration tests. The new CloudStorageWrapper correctly implements the ObjectStorageWrapper interface using the Google Cloud Storage client library. The PR also includes necessary refactoring in existing object storage classes to accommodate the new backend. I've found a couple of areas for improvement regarding exception handling and memory efficiency. Overall, this is a solid contribution.
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfig.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR backports Cloud Storage support to branch(3), adding a new object storage adapter implementation for Google Cloud Storage alongside the existing S3 and BlobStorage adapters.
Key changes:
- New Cloud Storage adapter implementation with configuration, wrapper, provider, and error handling classes
- Updated
ObjectStorageWrapper.close()interface signature to throwObjectStorageWrapperException - Refactored
getUsername()method fromObjectStorageConfiginterface to individual config implementations
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CloudStorageConfig.java | Configuration class for Cloud Storage with service account credentials handling |
| CloudStorageWrapper.java | Main wrapper implementation for Google Cloud Storage operations |
| CloudStorageProvider.java | Service provider implementation for Cloud Storage |
| CloudStorageErrorCode.java | Error code enum for Cloud Storage-specific errors |
| CloudStorageConfigTest.java | Unit tests for CloudStorageConfig |
| CloudStorageWrapperTest.java | Unit tests for CloudStorageWrapper |
| S3WrapperTest.java | Updated test method signature to include Exception |
| ObjectStorageWrapper.java | Updated close() method signature and added documentation |
| ObjectStorageConfig.java | Removed getUsername() from interface |
| S3Config.java/BlobStorageConfig.java | Moved getUsername() to concrete implementations |
| S3Wrapper.java | Updated close() to throw ObjectStorageWrapperException |
| ObjectStorageAdmin.java/ObjectStorage.java | Added error handling for close() method |
| PreconditionFailedException.java | Added constructor without cause parameter |
| ObjectStorageWrapperException.java | Added constructor without cause parameter |
| ObjectStorageWrapperFactory.java | Added Cloud Storage case to factory |
| ObjectStorageUtils.java | Added Cloud Storage case to config utility |
| DistributedStorageProvider | Registered CloudStorageProvider |
| CoreError.java | Added Cloud Storage error codes |
| build.gradle | Added google-cloud-storage dependency version |
| core/build.gradle | Added google-cloud-storage dependency and formatting fixes |
| ObjectStorageEnv.java | Added Cloud Storage environment check |
| ObjectStorageWrapperIntegrationTest.java | Updated for Cloud Storage list limits and version handling |
| ObjectStorageWrapperLargeObjectWriteIntegrationTest.java | Added Cloud Storage parallel upload configuration |
| object-storage-adapter-check.yaml | Added Cloud Storage CI/CD workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is an automated backport of the following:
Please merge this PR after all checks have passed.