-
Notifications
You must be signed in to change notification settings - Fork 40
Add support for Cloud Storage #3179
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
|
It looks like you might be trying to invoke a command. If you'd like me to perform a code review, please use |
...ation-test/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapperIntegrationTest.java
Show resolved
Hide resolved
...com/scalar/db/storage/objectstorage/ObjectStorageWrapperLargeObjectWriteIntegrationTest.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Show resolved
Hide resolved
|
/gemini review |
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 pull request introduces support for Google Cloud Storage as a backend for the Object Storage adapter. The changes include adding a new CloudStorageWrapper, its configuration, and corresponding unit and integration tests. The implementation is comprehensive and follows the existing patterns for other object storage providers. I've found a few issues, including a potential NullPointerException if credentials aren't provided, incorrect exception handling in batch deletes, a copy-paste error in the CloudStorageProvider, and a minor issue in the build configuration. Overall, this is a great addition, and with these fixes, it will be a robust implementation.
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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 pull request introduces support for Google Cloud Storage to the Object Storage adapter, which is a significant and well-executed enhancement. The changes include a new CloudStorageWrapper implementation, its corresponding configuration class, and a service provider. A notable improvement is the refactoring of the ObjectStorageConfig interface, which enhances abstraction and has been consistently applied to the existing Blob Storage and S3 implementations. Error handling has been improved, particularly in the close() methods of ObjectStorage and ObjectStorageAdmin. The addition of comprehensive unit and integration tests for the new Cloud Storage functionality is commendable. Overall, the pull request is of high quality. I have one suggestion to refine the exception handling in the CloudStorageWrapper constructor for better clarity.
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
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 adds Google Cloud Storage support as a new object storage adapter for ScalarDB, implementing the ObjectStorageWrapper interface to enable Cloud Storage as a backend storage option alongside existing adapters (S3, Azure Blob Storage).
Key Changes:
- Implemented CloudStorageWrapper with full CRUD operations using Google Cloud Storage API
- Added configuration support via CloudStorageConfig using DatabaseConfig properties
- Extended ObjectStorageWrapper interface to support exceptions in close() method and added documentation for deleteByPrefix atomicity constraints
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| CloudStorageWrapper.java | Core implementation of Cloud Storage adapter with get, insert, update, delete operations using Google Cloud Storage client |
| CloudStorageConfig.java | Configuration class mapping DatabaseConfig properties to Cloud Storage settings (project ID, bucket, credentials) |
| CloudStorageProvider.java | Service provider implementation for Cloud Storage adapter discovery |
| CloudStorageErrorCode.java | Enum defining Cloud Storage HTTP error codes for error handling |
| CloudStorageWrapperTest.java | Comprehensive unit tests for CloudStorageWrapper operations |
| CloudStorageConfigTest.java | Unit tests for configuration validation and property loading |
| ObjectStorageWrapper.java | Updated interface with close() exception handling and deleteByPrefix documentation |
| ObjectStorageWrapperException.java | Added constructor accepting message-only for improved exception handling |
| PreconditionFailedException.java | Added constructor accepting message-only for simpler exception creation |
| ObjectStorageConfig.java | Removed getUsername() method from interface (moved to implementation classes) |
| S3Config.java | Moved getUsername() from interface override to implementation-specific method |
| BlobStorageConfig.java | Moved getUsername() from interface override to implementation-specific method |
| ObjectStorageWrapperFactory.java | Added factory support for CloudStorageWrapper instantiation |
| ObjectStorageUtils.java | Added utility support for CloudStorageConfig creation |
| ObjectStorage.java | Updated close() to handle ObjectStorageWrapperException with logging |
| ObjectStorageAdmin.java | Updated close() to handle ObjectStorageWrapperException with logging |
| ObjectStorageWrapperIntegrationTest.java | Added Cloud Storage support and test data adjustments for numeric version strings |
| ObjectStorageWrapperLargeObjectWriteIntegrationTest.java | Added Cloud Storage parallel upload configuration and test data size adjustment |
| ObjectStorageEnv.java | Added isCloudStorage() helper for test environment detection |
| core/build.gradle | Added google-cloud-storage dependency and formatting improvements |
| build.gradle | Added googleCloudStorageVersion property (2.60.0) |
| .github/workflows/object-storage-adapter-check.yaml | Added Cloud Storage integration test workflow and fixed S3 artifact naming |
| META-INF/services/com.scalar.db.api.DistributedStorageProvider | Registered CloudStorageProvider for service discovery |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...com/scalar/db/storage/objectstorage/ObjectStorageWrapperLargeObjectWriteIntegrationTest.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapper.java
Outdated
Show resolved
Hide resolved
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/CloudStorageConfig.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageProvider.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Show resolved
Hide resolved
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
Torch3333
left a comment
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.
LGTM, thank you!
I just left non-blocking comments.
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
brfrn169
left a comment
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.
Left a minor comment. Other than that, LGTM! Thank you!
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
19a2e21 to
0d2bce7
Compare
feeblefakie
left a comment
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.
LGTM! Thank you!
komamitsu
left a comment
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.
LGTM! 👍
josh-wong
left a comment
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.
I left a minor suggestion and a comment (the comment isn't a blocker). Other than that, LGTM. Thank you!🙇🏻♂️
Description
This PR adds Cloud Storage support to the Object Storage adapter.
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
Added Google Cloud Storage adapter.