-
Notifications
You must be signed in to change notification settings - Fork 40
Add support for S3 #3141
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
Add support for S3 #3141
Conversation
|
/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 S3 support for the Object Storage adapter, which is a significant feature addition. The implementation is comprehensive, leveraging the AWS CRT-based S3 client for better performance and includes new configuration options, exception types, and both unit and integration tests. My review has identified a critical issue in the delete method's logic that needs to be addressed. Additionally, I've pointed out several areas for improvement regarding code duplication, potential memory inefficiencies when handling large numbers of objects, and making exception handling more robust. Overall, this is a solid contribution, and addressing these points will enhance its correctness and maintainability.
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Config.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfig.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.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 Amazon S3 support to the Object Storage adapter, enabling ScalarDB to use S3 as a backend storage option alongside the existing Azure Blob Storage adapter.
- Implemented
S3Wrapperclass that provides S3-specific operations (get, insert, update, delete, etc.) - Added configuration, provider, and error handling classes for S3
- Refactored shared exception constructors and interface methods to support both storage types
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| S3Wrapper.java | Core S3 implementation with async client operations and error handling |
| S3Config.java | Configuration parser for S3-specific settings (region, bucket, parallelism, etc.) |
| S3Provider.java | Service provider for S3 storage |
| S3ErrorCode.java | Enum for S3 HTTP status codes |
| S3WrapperTest.java | Comprehensive unit tests for S3Wrapper |
| S3ConfigTest.java | Unit tests for S3Config |
| ObjectStorageWrapperFactory.java | Added S3 instantiation logic |
| ObjectStorageUtils.java | Added S3 config factory logic |
| ObjectStorageConfig.java | Removed getEndpoint() method (S3-specific) |
| BlobStorageConfig.java | Moved getEndpoint() from interface to implementation, fixed error message |
| PreconditionFailedException.java | Removed unused single-argument constructor |
| ObjectStorageWrapperException.java | Removed unused single-argument constructor |
| ConflictOccurredException.java | New exception class for conflict errors |
| ObjectStorageWrapperIntegrationTest.java | Fixed test signature and improved test cleanup |
| ObjectStorageEnv.java | Added storage type configuration support |
| core/build.gradle | Added AWS S3 SDK dependencies |
| DistributedStorageProvider | Registered S3Provider service |
| object-storage-adapter-check.yaml | New GitHub workflow for S3 integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageEnv.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Config.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.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 several comments. PTAL!
...ation-test/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapperIntegrationTest.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageUtils.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapperFactory.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
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! 👍
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.
LGTM! Thank you!
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!
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.
Overall, looking good!
Left some suggestions. PTAL!
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Config.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
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!
Description
This PR adds S3 support to the Object Storage adapter.
Related issues and/or PRs
Changes made
ObjectStorageWrapperimplementation for S3.Checklist
Additional notes (optional)
N/A
Release notes
Added Amazon S3 adapter.