-
Notifications
You must be signed in to change notification settings - Fork 513
feat: add blob compaction support #5189
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
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5189 +/- ##
==========================================
- Coverage 82.05% 82.02% -0.03%
==========================================
Files 342 342
Lines 141516 141584 +68
Branches 141516 141584 +68
==========================================
+ Hits 116115 116134 +19
- Misses 21561 21612 +51
+ Partials 3840 3838 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wjones127
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.
It's unclear to me whether we need this flag. Was that discussed somewhere?
westonpace
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.
Can we drop the compact_blobs flag? I agree that compacting blobs right now is likely to lead to OOM due to the default batch size for compaction.
However, we have several efforts in progress which will fix this. For example, blob v2 will put these very large blobs in separate files so they are compacted with different logic.
Also, #4369 will change the default batch size so it works even if blobs are present.
I'm worried that we will add a flag (compact_blobs) and then, once the above features are done, we will need to remove the flag. I think it's ok to just move from "compacting blobs always errors" to "compacting blobs will probably run you out of memory unless you change the batch size".
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
|
Thank you @wjones127 and @westonpace for the review, I have removed the |
westonpace
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.
Just one outstanding question.
This is just the first step: the next steps include streaming blobs and adding a maximum limit to prevent OOMs.
This PR was primarily authored with Codex using GPT-5-Codex and then hand-reviewed by me. I AM responsible for every change made in this PR. I aimed to keep it aligned with our goals, though I may have missed minor issues. Please flag anything that feels off, I'll fix it quickly.