feat(flatkv): add snapshot, WAL catchup, and rollback support#2972
feat(flatkv): add snapshot, WAL catchup, and rollback support#2972blindchaser wants to merge 21 commits intomainfrom
Conversation
- Use getAccountValue (cache-aware) instead of getAccountValueFromDB for LtHash old-value reads, fixing incorrect deltas across multiple ApplyChangeSets calls before Commit - Clear pending write buffers in open() to prevent stale writes after LoadVersion - Fix misleading "always sync" comment on WAL writes (NoSync=true)
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bccc292045
ℹ️ 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".
| if err := s.open(); err != nil { | ||
| return fmt.Errorf("open for rollback: %w", err) | ||
| } |
There was a problem hiding this comment.
When Rollback updates current and immediately calls open, it does not invalidate working/SNAPSHOT_BASE, so after a restart (where working was already cloned from that same snapshot) createWorkingDir reuses a working DB that may already be at a higher version than targetVersion. In that case catchup skips all entries (entry.Version <= committedVersion), rollback fails with a version mismatch, and this can happen after the WAL has already been truncated, leaving rollback in a partially-mutated state. Force a fresh clone of working from the selected snapshot before opening during rollback (like LoadVersion(target>0) already does).
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| s.pruneSnapshots(dir, version) | ||
|
|
||
| success = true | ||
| s.lastSnapshotTime = time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2972 +/- ##
===========================================
+ Coverage 58.11% 66.84% +8.73%
===========================================
Files 2109 25 -2084
Lines 173402 1903 -171499
===========================================
- Hits 100765 1272 -99493
+ Misses 63684 432 -63252
+ Partials 8953 199 -8754
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // Checkpointable is an optional capability for DB engines that support | ||
| // efficient point-in-time snapshots via filesystem hardlinks. | ||
| type Checkpointable interface { |
There was a problem hiding this comment.
Can you augment the godoc with information about concurrency? For example, is it safe to call this method concurrently with updates in other threads? When this method returns, is the checkpoint capable of surviving a host OS crash?
| snapshotPrefix = "snapshot-" | ||
| snapshotDirLen = len(snapshotPrefix) + 20 | ||
|
|
||
| currentLink = "current" | ||
| currentTmpLink = "current-tmp" |
There was a problem hiding this comment.
Short descriptions for what each constant is for might be helpful.
There was a problem hiding this comment.
One way to document this, feel free to push back if you think it's too much overhead.
At past companies, when documenting this sort of file layout, I sometimes find it useful to do the following:
- write a simple unit test that generates a basic file structure
- pause the unit test before it deletes its data
- run
treeon the directory created by the test - edit the result for readability copy-paste the rest somewhere
Here's an example: https://github.com/Layr-Labs/eigenda/blob/master/litt/docs/filesystem_layout.md
If the file layout is to big, it might make sense to split it out into a markdown file that you can just reference in the godoc.
| // not a full path. | ||
| func updateCurrentSymlink(root, snapshotDir string) error { | ||
| tmpPath := filepath.Join(root, currentTmpLink) | ||
| _ = os.Remove(tmpPath) |
There was a problem hiding this comment.
What if this removal fails due to invalid file permissions? I'm guessing you aren't checking the error in case it fails due to the path not existing. Perhaps this can first check if the file exists, delete it if it exists, and then return an error if that deletion fails.
| // flatkv/ | ||
| // current -> snapshot-NNNNN | ||
| // snapshot-NNNNN/{account,code,...}/ (immutable) | ||
| // working/{account,code,...}/ (mutable clone) | ||
| // changelog/ (WAL, shared) |
There was a problem hiding this comment.
Ah, didn't see this when I made my comment above. It might be nice to replicate this to the other file that deals with directory names.
2 participants
Describe your changes and provide context
Introduce a snapshot-based lifecycle for FlatKV so that restarts replay
only from the nearest PebbleDB checkpoint instead of the full WAL.
Key changes:
Checkpoint(), managed through a "current" symlink and atomic
directory operations. Configurable interval, retention, and
minimum time between snapshots.
for .sst files) so writes never mutate snapshot dirs.
version to the target version using O(1) arithmetic + O(log N)
binary search for offset resolution.
replay to exact version, and prune future snapshots.
versioned snapshot directory on first open.
the earliest retained snapshot.
multiple ApplyChangeSets calls precede a single Commit.
Testing performed to validate your change