-
-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add S3 storage support for chunks #668
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 @kalbasit, 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 significantly enhances the storage capabilities by introducing an S3-compatible backend for content-defined chunks. This allows the system to store and retrieve data chunks from cloud storage services, providing a more distributed and resilient storage option. The implementation includes all necessary CRUD operations for chunks and is thoroughly tested with integration tests. 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 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 S3 storage support for chunks, which is a great feature. The implementation is clean and includes integration tests. I've identified a couple of areas for improvement: one related to making the S3 endpoint URL parsing more robust, and another concerning a race condition in the PutChunk method, which is acceptable for now given the current application context and documentation as technical debt. My comments provide specific suggestions to address these points.
1643466 to
8bb122a
Compare
844b061 to
8b0a778
Compare
8bb122a to
29bc630
Compare
29bc630 to
b92988e
Compare
b92988e to
e7a90b5
Compare
7a370b5 to
a033c74
Compare
2a45a7a to
736ed2e
Compare
736ed2e to
87d7b35
Compare
|
/gemini review |
1f9be6a to
ef1a6b7
Compare
b028cf2 to
29af693
Compare
ef1a6b7 to
094773e
Compare
27650ba to
3066fbd
Compare
da6a58a to
721eab8
Compare
e7890b6 to
64c004f
Compare
64c004f to
7983dd4
Compare
|
/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-compatible storage for content-defined chunks, which is a great addition. The implementation in pkg/storage/chunk/s3.go is well-structured, using the MinIO Go SDK and incorporating locking to prevent race conditions during chunk uploads. The integration tests are comprehensive and cover the main use cases, including race conditions.
I have a couple of suggestions to improve maintainability and robustness in the S3 store implementation. One is to avoid a hardcoded TTL for locks, and the other is to make the error handling for non-existent chunks more consistent. Please see the detailed comments.

This commit adds a new S3-compatible storage backend for content-defined chunks:
Part of #322