Skip to content
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.

Sign up for GitHub

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

Jump to bottom

levels: Expired keys and delete markers are never purged #1354

Merged
merged 4 commits into from
Jun 2, 2020
Merged

levels: Expired keys and delete markers are never purged #1354

merged 4 commits into from
Jun 2, 2020

Conversation

damz
Copy link
Contributor

@damz damz commented Jun 2, 2020 •
edited by manishrjain
Loading

The compaction process accidentally keeps one more version of expired keys and delete markers than it should, because of logic error in *levelsController.compactBuildTables(). When NumVersionsToKeep is 1, it means that expired keys and delete markers are never actually purged from the LSM tables.

This change is Reviewable

@damz
Copy link
Contributor Author

damz commented Jun 2, 2020

Unfortunately, this is really tricky to test, because I don't see a way to force the engine to purge the memtable. I forced a close/open of the database in the test, which does the trick but it is not extremely pretty.

@jarifibrahim
Copy link
Contributor

@damz The delete markers issue should be fixed by #1303

@damz damz force-pushed the pr/discard-expired branch from 56cbfba to 1a3c16b Compare June 2, 2020 13:49
@damz
Copy link
Contributor Author

damz commented Jun 2, 2020

@jarifibrahim Thank you for pointing out the obvious :)

I cherry-picked the test from the other PR, which looks much better than my attempt. I confirm that it fails before the fix and passes after.

jarifibrahim
jarifibrahim approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: I've pushed some more tests @damz! Thank you for fixing this.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

@jarifibrahim jarifibrahim mentioned this pull request Jun 2, 2020
Closed
ou05020
ou05020 reviewed Jun 2, 2020
View reviewed changes
Copy link
Contributor

@ou05020 ou05020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

ou05020
ou05020 reviewed Jun 2, 2020
View reviewed changes
Copy link
Contributor

@ou05020 ou05020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

martinmr
martinmr approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

manishrjain
manishrjain approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Nice catch!

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami)

@jarifibrahim jarifibrahim merged commit fd89894 into dgraph-io:master Jun 2, 2020
jarifibrahim pushed a commit that referenced this pull request Jul 8, 2020
Update dgraph-maintenance branch
3b7d067 
This PR adds the following changes from **Master branch to the dgraph-maintenance branch**.
```
25fd0ef Update ristretto to commit f66de99 (#1391)
fac972f Update head while replaying value log (#1372)
e5fd05a Rework DB.DropPrefix (#1381)
adeb842 Fix assert in background compression and encryption. (#1366)
```

The **master and dgraph-maintenace branch** have following difference 
`+`  (in green) means commit missing on dgraph-maintenance
 `-` (in red) means the commit exists.
```diff
+ 079f5ae DefaultOptions: Set KeepL0InMemory to false (#1345)
+ 7e19cac Add immudb to the project list (#1341)
+ da80eb9 Iterator: Always add key to txn.reads (#1328)
+ a7e239e StreamWriter: Close head writer (#1347)
+ 543f353 Fix build on golang tip (#1355)
+ fd89894 Compaction: Expired keys and delete markers are never purged (#1354)
+ 056d859 Support disabling conflict detection (#1344)
+ b762832 Tests: Do not leave behind state goroutines (#1349)
+ b2267c2 Restore: Account for value size as well (#1358)
+ 14386ac GC: Consider size of value while rewriting (#1357)
+ c45d966 Fix assert in background compression and encryption. (#1366)
+ dd332b0 Avoid panic in filltables() (#1365)
+ 3f4761d Force KeepL0InMemory to be true when InMemory is true (#1375)
+ d37ce36 Tests: Use t.Parallel in TestIteratePrefix tests  (#1377)
+ 158d927 Remove second initialization of writech in Open (#1382)
+ 675efcd Increase default valueThreshold from 32B to 1KB (#1346)
+ 3042e37 pre allocate cache key for the block cache and the bloom filter cache (#1371)
+ e013bfd Rework DB.DropPrefix (#1381)
+ 509de73 Update head while replaying value log (#1372)
+ 09dfa66 Update ristretto to commit f66de99 (#1391)
+ 717b89c Enable cross-compiled 32bit tests on TravisCI (#1392)
```

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/dgraph-io/badger/1398)
<!-- Reviewable:end -->
@jarifibrahim jarifibrahim mentioned this pull request Jul 12, 2020
Merged
NamanJain8 pushed a commit that referenced this pull request Sep 8, 2020
@damz @NamanJain8
Compaction: Expired keys and delete markers are never purged (#1354)
994905f 
The compaction process accidentally keeps one more version of
expired keys and delete markers than it should, because of
logic error in *levelsController.compactBuildTables(). When
NumVersionsToKeep is 1, it means that expired keys and delete
markers are never actually purged from the LSM tables.

Co-authored-by: Julian Hegler <julian.hegler@tanium.com>
Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
(cherry picked from commit fd89894)
NamanJain8 added a commit that referenced this pull request Sep 9, 2020
@NamanJain8 @damz
Compaction: Expired keys and delete markers are never purged (#1354) (#…
dd51dcf 
…1510)

The compaction process accidentally keeps one more version of
expired keys and delete markers than it should, because of
logic error in *levelsController.compactBuildTables(). When
NumVersionsToKeep is 1, it means that expired keys and delete
markers are never actually purged from the LSM tables.

Co-authored-by: Julian Hegler <julian.hegler@tanium.com>
Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
(cherry picked from commit fd89894)

Co-authored-by: Damien Tournoud <damien@platform.sh>
jarifibrahim pushed a commit that referenced this pull request Oct 2, 2020
@damz @ou05020
Compaction: Expired keys and delete markers are never purged (#1354)
09dc827 
The compaction process accidentally keeps one more version of
expired keys and delete markers than it should, because of
logic error in *levelsController.compactBuildTables(). When
NumVersionsToKeep is 1, it means that expired keys and delete
markers are never actually purged from the LSM tables.

Co-authored-by: Julian Hegler <julian.hegler@tanium.com>
Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
@RubenKelevra RubenKelevra mentioned this pull request Mar 24, 2021
Open
@avinashparchuri avinashparchuri mentioned this pull request Jun 13, 2022
Merged
mYmNeo pushed a commit to mYmNeo/badger that referenced this pull request Jan 16, 2023
@NamanJain8 @damz @mYmNeo
Compaction: Expired keys and delete markers are never purged (dgraph-…
94eaa4a 
…io#1354) (dgraph-io#1510)

The compaction process accidentally keeps one more version of
expired keys and delete markers than it should, because of
logic error in *levelsController.compactBuildTables(). When
NumVersionsToKeep is 1, it means that expired keys and delete
markers are never actually purged from the LSM tables.

Co-authored-by: Julian Hegler <julian.hegler@tanium.com>
Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
(cherry picked from commit fd89894)

Co-authored-by: Damien Tournoud <damien@platform.sh>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ou05020 ou05020 ou05020 left review comments

@jarifibrahim jarifibrahim jarifibrahim approved these changes

@manishrjain manishrjain manishrjain approved these changes

@martinmr martinmr martinmr approved these changes

@ashish-goswami ashish-goswami Awaiting requested review from ashish-goswami

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@damz @jarifibrahim @martinmr @manishrjain @ou05020