Repository navigation
flatkv uses mutations - #4665
Conversation
PR SummaryHigh Risk Overview The view engine’s LtHash hashing is simplified: the gather phase that diffed sealed views and fetched prior values is gone; Tests and recovery scenarios were updated to commit through the mutation helpers and to simulate per-DB skew by copying/restoring data DB snapshots rather than rewinding version metadata. Reviewed by Cursor Bugbot for commit be4db01. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4665 +/- ##
==========================================
- Coverage 60.05% 59.95% -0.10%
==========================================
Files 2020 2017 -3
Lines 158499 157902 -597
==========================================
- Hits 95187 94673 -514
+ Misses 63307 63224 -83
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR changes flatKV so each block's writes and its lattice hash both come from one list of mutations (each carrying the key's previous and new value). That replaces the view manager's BatchUpdate folds and the hash engine's separate gather phase, and both public commit entry points now share one commitMutations path. I traced the account merge, empty-versus-nil values, import translation, replay skip lists, WAL slice ownership and hash-engine shutdown against the base branch and found no correctness defect, so nothing blocks; the codex reading (no findings) agrees with this and contributed nothing further.
Non-blocking
- The lattice hash used to be computed from values read back out of the store; it is now computed from the PreviousValue each caller supplies (from MutationsFromChangeSets or from the WAL record), and nothing checks it against the store. Replay therefore no longer corrects itself when a database's rows are ahead of its recorded version: such a block is now hashed twice instead of being a no-op. Correctness now depends on each database writing its rows and its metadata in one batch, and on requireAlignedDataDBs refusing to open nil-WAL stores whose databases are at different heights. The tests that pinned the old idempotency (TestTornCommitStillRecoversFromWAL, TestCatchupReplaysAlreadyAppliedBlockOnSeededStore, and the version-only rewinds in the skew tests) were deleted or rewritten to restore whole databases. Consider a cheap test-build or debug check that a mutation's PreviousValue matches the prior view, so a caller mistake fails loudly instead of quietly forking the AppHash.
seidroid review · decision approve · session 129209995f3145d9bed8a398eba07a79 · turn resp_claude_1bb4bb60510ca9855c6dcdbca8ae4507 · item 160b973fe3475a779e28542a5dd50e27
Findings: 0 blocking | 1 non-blocking | 0 posted inline
| return nil | ||
| writes := make([]view.Write, len(mutations)) | ||
| for i := range mutations { | ||
| writes[i] = view.Write{Key: string(mutations[i].Key), Value: mutations[i].NewValue} |
There was a problem hiding this comment.
Nit: This could be 10-20k allocations per superblock. Mutation keys are never modified, and the view only keeps the key string until the version retires (it clones on first insert into the long-lived map), so unsafe.String would avoid the copy.
…recompile Adapt the flatkv test store to CommitStateChanges taking mutations (#4665).
Refactor flatKV to utilize mutations. Dramatically reduces internal complexity of flatKV, and eliminates a lot of duplicate work.