Repository navigation
fix(posting): set minTs for empty rollups, drop replayed commits belo… - #9863
shiva-istari wants to merge 1 commit into
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately identifies the main fix: setting minTs for empty rollups and dropping replayed commits below that threshold. Linked Issues check The changes satisfy the coding requirements in [ #9856].ReadPostingListsetsList.minTstoitem.Version()forBitEmptyPosting, so reads before the delete can use older deltas while reads at or…Out of Scope Changes check All production changes and tests are connected to [ #9856]. The cache invalidation change supports replay safety for rolled-up posting lists. The unit and integration tests provide regression coverage …Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements in [
#9856].ReadPostingListsetsList.minTstoitem.Version()forBitEmptyPosting, so reads before the delete can use older deltas while reads at or after the marker see an empty list.MemoryLayer.updateItemInCachedrops cached lists when a replayed commit hascommitTs <= minTs, which prevents the cache error and allows disk rebuild. The MVCC tests cover earlier snapshots and replayed commits for empty and complete rollups. The integration tests cover UID edges,count(),has()filters, and incremental backup/restore behavior.
Full details: Out of Scope Changes check
-
Explanation
All production changes and tests are connected to [
#9856]. The cache invalidation change supports replay safety for rolled-up posting lists. The unit and integration tests provide regression coverage for the corrected snapshot and backup/restore behavior. No unrelated changes are shown in the reviewed diff.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
This comment has been minimized.
This comment has been minimized.
…w minTs When a delete empties a posting list, the incremental rollup rewrites the key as a BitEmptyPosting marker at commitTs+1. ReadPostingList did not set List.minTs for that marker (it does for BitCompletePosting), so: - on a cache miss MemoryLayer.ReadData, which reads the key at math.MaxUint64, served the empty list to every read ts, including transactions that started before the delete; and - ToBackupPostingList versioned the emptied key at 0, so restoring a full plus an incremental backup kept the older, non-empty version. Set minTs = item.Version() for BitEmptyPosting as well. The line was dropped in ee99a7a (#9237) and the bug first shipped in v24.1.0. With minTs set, a commit replayed from the Raft log after a restart can reach updateItemInCache with commitTs <= minTs of the cached list, because the rollup that already contains it is versioned newest commit + 1. Folding it in again puts a delta below minTs into the mutable layer, and if it is a delete-all every read the entry serves fails with "deleteBelowTs must be greater than the minTs in the list" until the entry is evicted. Drop the cache entry in that case instead; the next read rebuilds it from disk. This path was already reachable for complete rollups before this change. Tests: posting/mvcc_test.go gains the reporter's snapshot-read test and a replay test for both rollup kinds; systest/integration2/rollup_empty_list_test.go reproduces the earlier-start-ts read and the backup/restore case end to end. Fixes #9856
59e3f50 to
9c7d5fa
Compare
| deleteAllFriends(t, gc, uids) | ||
| require.Equal(t, 0, countNodesWithFriend(t, gc)) | ||
|
|
||
| // Give the rollup time to rewrite the emptied keys. |
There was a problem hiding this comment.
I think these can pass without the fix. Nothing checks that the rollup actually landed before the incremental backup, and if it hasn't, the backup carries the delete as a delta, which ToBackupPostingList versions correctly anyway. Same with TestReadAtEarlierStartTsAfterDeleteEmptiesList: if the rollup is late, every round reads the deltas and passes.
Did you confirm both go red against main? At ~30s of fixed waits per test I'd like to know they're earning it. I couldn't find a rollup metric to poll on either, so I get why the sleeps are here.
There was a problem hiding this comment.
Hi @matthewmcneely , I confirmed both tests go red on main (built no-fix and fix binaries, ran both against each). But you're right it's not guaranteed every run. Incremental rollup is lossy and cooldown-gated — skipped keys get dropped from the batch and never retried — so the emptied keys get ~one rollup attempt and only a few land (I saw ~3–4 regardless of node count). A run where none roll up would pass on buggy main too; fewer keys makes that more likely, which is why I bumped the count. A longer sleep doesn't help — the dropped keys are never retried.
The real fix is a signal to wait on, like WaitForSnapshot. There's none for rollup today. it's local and unreported. incrRollupi already counts rollups (ir.count), just not exposed.
Separate PR: expose a rollups_total counter (one ostats.Record where ir.count increments in rollUpKey + a View in x/metrics.go) and a dgraphapi.WaitForRollup, so these tests wait on the counter instead of sleeping. Thoughts?
…w minTs
Fixes #9856
When a delete empties a posting list, the incremental rollup rewrites the key as a BitEmptyPosting marker at commitTs+1. ReadPostingList did not set List.minTs for that marker (it does for BitCompletePosting), so:
Set minTs = item.Version() for BitEmptyPosting as well. The line was dropped in ee99a7a (#9237) and the bug first shipped in v24.1.0.
With minTs set, a commit replayed from the Raft log after a restart can reach updateItemInCache with commitTs <= minTs of the cached list, because the rollup that already contains it is versioned newest commit + 1. Folding it in again puts a delta below minTs into the mutable layer, and if it is a delete-all every read the entry serves fails with "deleteBelowTs must be greater than the minTs in the list" until the entry is evicted. Drop the cache entry in that case instead; the next read rebuilds it from disk. This path was already reachable for complete rollups before this change.
Tests: posting/mvcc_test.go gains the reporter's snapshot-read test and a replay test for both rollup kinds;
systest/integration2/rollup_empty_list_test.go reproduces the earlier-start-ts read and the backup/restore case end to end.
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit