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.

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

Do not keep old key versions on disk in Badger #2150

Conversation

mschneider82
Copy link

@mschneider82 mschneider82 commented Apr 3, 2020

Which problem is this PR solving?

Resolves #1916

Short description of the changes

set NumVersionsToKeep to 0 instead of default 1 acording to dgraph-io/badger#1228

@mschneider82 mschneider82 requested a review from a team as a code owner April 3, 2020 08:28
@pavolloffay
Copy link
Member

@mschneider82 could you please sign off the PR? (DCO check)

@pavolloffay pavolloffay added the storage/badger Issues related to badger storage label Apr 3, 2020
@pavolloffay pavolloffay changed the title fix #1916: dont keep old versions on disk Do not keep old key versions on disk in Badger Apr 3, 2020
@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #2150 into master will increase coverage by 0.01%.
The diff coverage is 78.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2150      +/-   ##
==========================================
+ Coverage   96.14%   96.16%   +0.01%     
==========================================
  Files         219      219              
  Lines       10585    10586       +1     
==========================================
+ Hits        10177    10180       +3     
+ Misses        353      351       -2     
  Partials       55       55              
Impacted Files Coverage Δ
cmd/collector/app/server/http.go 0.00% <0.00%> (ø)
cmd/collector/app/server/zipkin.go 0.00% <0.00%> (ø)
cmd/flags/service.go 0.00% <0.00%> (ø)
cmd/flags/admin.go 79.36% <83.33%> (ø)
cmd/collector/app/builder_flags.go 100.00% <100.00%> (ø)
cmd/collector/app/collector.go 69.11% <100.00%> (ø)
cmd/collector/app/server/grpc.go 65.38% <100.00%> (ø)
plugin/storage/badger/factory.go 98.14% <100.00%> (+0.01%) ⬆️
ports/ports.go 100.00% <100.00%> (+100.00%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87b1910...2a31ad7. Read the comment docs.

@objectiser
Copy link
Contributor

@burmanm Any concerns with this change?

@burmanm
Copy link
Contributor

burmanm commented Apr 3, 2020

It's not really doing what I think the author thinks it's doing:

```// This is important. Without this Badger would keep atleast 1 version of very key.``

In reality, NumVersionsKeep is documented as: NumVersionsToKeep sets how many versions to keep per key at most. so it's a maximum, not minimum. Also, based on the code in the levels.go I don't see why it would make a difference in 1 vs 0:

https://github.com/dgraph-io/badger/blob/eaf64c0925f5a62d532986db526ed2c571cb22df/levels.go#L579

So if this helps with cleaning, it sounds like a bug in the badger rather than correct configuration option.

@mschneider82
Copy link
Author

mschneider82 commented Apr 3, 2020

in dgraph-io/badger#1228 they recommend to set it to 0 to solve the issue, but that issue is still open, lets see how the default may change. I think instant drop is what we want for jaeger, so 0 is the setting for instant drop after expired (GC).

@CsterKuroi
Copy link

Looks nice to me.

@colinhoglund
Copy link

FYI, according to this closed PR (dgraph-io/badger#1300), changing this to 0 might have unintended effects on valid entries (dgraph-io/badger#1300 (comment)).

@jarifibrahim
Copy link

Hey! Please do not set number of versions to zero. Please see dgraph-io/badger#1228 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
storage/badger Issues related to badger storage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Badger not cleaning data in expected manner
7 participants