-
Notifications
You must be signed in to change notification settings - Fork 40
Backport to branch(3) : Add support for S3 #3175
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 integrates Amazon S3 as a new object storage option, expanding the system's compatibility with cloud storage providers. It involves adding S3-specific components, adapting the existing object storage interfaces and utilities for multi-storage support, and refining error handling and configuration mechanisms to ensure robust and flexible operation across different object storage backends. 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 pull request introduces support for Amazon S3 as a backend for the object storage component. The changes are comprehensive, including the addition of S3-specific configuration and wrapper classes, updates to factory classes, and modifications to integration tests to accommodate S3. The implementation is well-structured. I have identified a potential issue in an integration test related to large object uploads and a minor refactoring opportunity in the exception handling logic of the new S3Wrapper class. Overall, this is a solid contribution to extend the object storage capabilities.
...com/scalar/db/storage/objectstorage/ObjectStorageWrapperLargeObjectWriteIntegrationTest.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.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 S3 support for ScalarDB, adding the ability to use Amazon S3 as an object storage backend alongside the existing Azure Blob Storage support. This is an automated backport from the original PR #3141.
Key Changes:
- Added S3 storage adapter implementation with configuration, wrapper, provider, and error handling classes
- Modified object storage exception hierarchy to require a cause parameter and added ConflictOccurredException
- Updated BlobStorageConfig to make performance-related configuration options optional and return Optional values
- Added comprehensive unit and integration tests for S3 support
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| S3Wrapper.java | Main S3 wrapper implementation using AWS SDK S3AsyncClient with async operations |
| S3Config.java | Configuration class for S3 storage connection and performance settings |
| S3Provider.java | Provider implementation to register S3 as a storage type |
| S3ErrorCode.java | Enum mapping HTTP status codes to S3 error conditions |
| S3WrapperTest.java | Comprehensive unit tests for S3Wrapper functionality |
| S3ConfigTest.java | Unit tests for S3Config validation and property loading |
| BlobStorageConfig.java | Updated to make performance options optional, consistent with S3 implementation |
| BlobStorageWrapper.java | Updated to handle optional performance configuration values |
| ObjectStorageWrapperException.java | Removed no-cause constructor to enforce proper error context |
| ConflictOccurredException.java | New exception type for conflict errors (409 status) |
| MutateStatementHandler.java | Updated to catch ConflictOccurredException as retriable error |
| ObjectStorageEnv.java | Added S3 configuration support to integration test environment |
| object-storage-adapter-check.yaml | New GitHub Actions workflow for S3 integration testing |
| core/build.gradle | Added AWS SDK S3 and CRT client dependencies |
💡 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.