-
-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add CLI and Helm configuration for CDC and S3 #670
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
feat: add CLI and Helm configuration for CDC and S3 #670
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 expands the application's configurability by integrating Content-Defined Chunking (CDC) and S3 storage options. Users can now enable and fine-tune CDC for deduplication and specify S3 as a storage backend for chunks, all manageable through new CLI flags and updated Helm chart values. This change aims to provide greater flexibility in deployment and storage management, particularly for experimental deduplication features. 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 configuration for Content-Defined Chunking (CDC) and S3 storage through both CLI flags and Helm chart values. The changes are well-structured, exposing the new functionality consistently across different configuration methods. My review notes the added validation in pkg/ncps/serve.go for S3 configuration, which is a positive improvement. The observed code duplication is acceptable based on project guidelines. Overall, this is a good addition.
2e22ada to
d31e2e7
Compare
24941e7 to
aad1081
Compare
ffe4427 to
1306a80
Compare
e20990e to
be66a95
Compare
d9b7e61 to
2a652d4
Compare
508ea4b to
4ac6f9b
Compare
1628095 to
9254a8c
Compare
64f7254 to
a4e6430
Compare
9254a8c to
8870a06
Compare
a4e6430 to
e3253d1
Compare
0cdaa05 to
1359098
Compare
f56283d to
a1cfc79
Compare
33c24d3 to
ae9f0bf
Compare
b1ea8a4 to
bd319b4
Compare
ae9f0bf to
4f09e4e
Compare
This commit exposes content-defined chunking (CDC) and S3 storage settings to users: - Added CLI flags for enabling CDC and configuring chunk size boundaries (min/avg/max). - Added CLI flags for S3 storage configuration (bucket, endpoint, region, etc.). - Updated the Helm chart with new values and ConfigMap templates for CDC and S3. - Provided a complete CDC configuration example in config.example.yaml.
4f09e4e to
f283f3d
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 configuration for Content-Defined Chunking (CDC) and S3 storage via CLI flags and Helm chart values. The changes are well-structured, adding the necessary flags, updating the Helm chart templates and values, and providing an example in config.example.yaml.
My main feedback is regarding code duplication in pkg/ncps/serve.go. The new getChunkStorageBackend function repeats logic from existing storage configuration functions, which has resulted in missing validation for conflicting storage settings and incomplete S3 configuration. I've left a comment with a suggestion to fix this.
|
/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 configuration for Content-Defined Chunking (CDC) and S3 storage through both CLI flags and Helm chart values. The changes are well-structured, particularly the refactoring in pkg/ncps/serve.go which centralizes storage configuration logic into a new getStorageConfig function. This improves code clarity and maintainability.
My review includes a couple of suggestions for improvement:
- Removing redundant validation for S3 configuration.
- Adding validation for CDC chunk size parameters to prevent potential issues with negative inputs.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
=======================================
Coverage 79.16% 79.16%
=======================================
Files 2 2
Lines 456 456
=======================================
Hits 361 361
Misses 83 83
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Related Documentation No published documentation to review for changes on this repository. |

This commit exposes content-defined chunking (CDC) and S3 storage settings to users:
Part of #322