fix(compaction): Don't drop data when split overlaps with top tables #1587
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
November 5, 2020 14:09
There was a race condition in the subcompaction code which was caused because we were doing builder.Close after doing throttle.Done in subcomapction. This PR fixes that race condition by calling builder.Done before calling throttle.Done .
added 7 commits
November 6, 2020 17:13
… tables" This reverts commit 8957ad4.
manishrjain
reviewed
Nov 6, 2020
| // This will generate splits like [A1 ... C2] . Notice that we | ||
| // dropped the C3 which is the last key of the top table. | ||
| // See TestCompaction/with_split test. | ||
| right := y.KeyWithTs(y.ParseKey(t.Biggest()), math.MaxUint64) |
There was a problem hiding this comment.
getKeyRange takes care of these things. I thought I was using it -- that's the reason for the bug.
In fact, don't we want to consider the right most key? In that case, the right should be using 0 as the version.
There was a problem hiding this comment.
We're generating key ranges using the bot tables so they don't have the correct key version.
In fact, don't we want to consider the right most key? In that case, the right should be using 0 as the version.
No. We skip the right keys of the key range.
https://github.com/dgraph-io/badger/blob/6c074551f8f5a5b470357236cd90e860d0deb517/levels.go#L666-L668
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Badger compactions are split into sub compactions if there are more than 3 tables
bottom tables are involved. The current implementation would drop keys if the top
table would overlap with a bottom table. Consider the following scenario
When this range is compacted, the result was
[ A B C(valid) D ](only
C(deleted)was dropped). The expected result was[A B D].This PR fixes the above-mentioned issue. The test in this PR fails on master
but works with this change.
This change is