-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Refactor: Write all queue state to a single key on each write for reliable backup #13047
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
base: main
Are you sure you want to change the base?
Conversation
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.
Before starting to always write the size (there is a benefit that we never written it) we need to:
- Decide if we also need to write the type of the size or we have 3 different sizes materialized on disk.
- What to do when user changes the sizer type and we don’t know the size when loading metadata, the current implementation is a bit broken because it needs to have a full drain until size is restored.
- Should we force full drain instead during shutdown or init without a known size?
Can you please do a PR that only adds the new proposed way (without the replacement or connection) then the second PR to start using that. As you saw there are some questions about this that I would like to address in the first PR, and also some questions about transition. |
Certainly, I can do that. I'm happy to proceed with the two-PR approach as you've outlined, allowing us to address your questions and concerns regarding the new method and the transition in a structured way. |
The first PR has been created. #13067 |
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (75.71%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #13047 +/- ##
==========================================
- Coverage 91.53% 91.36% -0.18%
==========================================
Files 504 504
Lines 28154 28384 +230
==========================================
+ Hits 25772 25933 +161
- Misses 1873 1923 +50
- Partials 509 528 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Are you planning to update this PR to use already merged code?
pq.initPersistentContiguousStorage(ctx) | ||
// Make sure the leftover requests are handled | ||
pq.retrieveAndEnqueueNotDispatchedReqs(ctx) | ||
pq.mu.Unlock() |
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.
Use defer
I'm currently finalizing the unit tests and getting ready to update the PR. |
The second PR has been created #13126 , PTAL. |
Fixes #12890