-
Notifications
You must be signed in to change notification settings - Fork 53
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
Introduce a recovery strategy as a replacement for set_discard_if_corrupted #235
Conversation
SIGABRTs for the mac tests. yey, gonna need to look into that. |
SIGABRTs gone now, with latest Rust (1.68.2, released hours ago). I suspect some resource exhaustion, which happens to be gone now? 🤷 |
(the one remaining called out by github is on the push, not the pull request. #236 is taking care of not launching all those CI runs on push) |
c473c37
to
9c1f27b
Compare
(Closing and reopening to retrigger the CI) |
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.
Looks good. The build failures are because of log
doing MSRV bump without a corresponding major version bump.
RecoveryStrategy::Discard => Ok((DatabaseArena::new(), HashMap::new())), | ||
RecoveryStrategy::Rename => { | ||
let corrupted_path = path.with_extension(DEFAULT_CORRUPT_DB_EXTENSION); | ||
fs::rename(path, corrupted_path)?; |
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.
What if the file already exists because of the previous corruption? Can we have a timestamp on the filename to prevent collision?
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.
I guess the assumption was that one deals with the $db.corrupt
file soon, before hitting another problem. But I can see that that's not always possible.
If we use timestamps rkv-users need to make sure to actually clean up to avoid piling up old corrupted files.
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.
By using Rename I'd expect the user to deal with the files, indeed. I wonder what they can really do with it if rkv can't do anything, though.
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.
For debugging purporses someone could manually explore that file and see if they are able to detect why the file got corrupted.
I thought about the .corrupt
vs .$timestamp
option again and I would say we should stay with .corrupt
for now:
- It's what's been discussed a while back
- It avoids piling up files over files
- The logic to find it is kept simple
- We still have the option in the future to change it. Let's see if it's even gonna be adopted inside m-c
BTW, sorry for the huge delay, this just slipped. I hope there's a central Mozilla dashboard to manage both Phabricator and GitHub 😅 Or at least a separate GitHub dashboard for my paid job... |
No worries. I guess no one ever really pushed for this anyway. |
I addresses the first few things, but I need to think about the filename part a bit more. |
9c1f27b
to
e055fb8
Compare
e055fb8
to
e47131c
Compare
…rupted The user can now choose between discarding corrupted data, moving it out of the way (into another file) and starting with an empty database or bubbling up the error. Fixes #234
e47131c
to
3ed758b
Compare
Nice, r+ 👍 (I can't reapprove that I already did, apparently 😄) |
Fixes #234
This is a breaking change because of the now-changed public API