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

Patch in https://github.com/dgraph-io/badger/pull/1313 to fix GC after restore. #2

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

avinashparchuri
Copy link

@avinashparchuri avinashparchuri commented Jun 11, 2022

This is a patch of dgraph-io#1313; While the original patch indicates that the fix should only be applicable when oldOptions.ValueThreshold < newOptions.ValueThreshold, we seem to hit this case in other situations as well, this patch fixes GC on our restored DBs (I've also verified the # of doc-ids per-corpus after restore).

I've been unable to reproduce this case in our unit-tests (i.e keeping ValueThreshold the same between backup/restore) - my suspicions are on some interaction between GC and the restore, but this has been hard to replicate/reproduce.

The other option is to just upgrade to mainline v2.2007 - it looks like it contains a bunch of other fixes that we probably want as well (and which might also be playing a part in causing these inconsistent value-pointer bits):

It is a bit more risky to upgrade (ex: we may see a recurrence of the GC issues; mainline v2.2007 is still using an older version of Ristretto, whereas our fork seems to have been upgraded), especially given that this will be the first time we will be exercising the restore path for prod-instances, but we could save time and just do the upgrade now, rather than having to revisit later.

Copy link

@asimshankar asimshankar left a comment

Choose a reason for hiding this comment

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

Cool find!
My gut feeling is that ideally we'd upgrade only after we've been able to successfully been able to restore existing backups. That way we'd have the ability to at least go back to backup if the upgrade causes trouble.

However, not sure if we're going to get to that state :). 🤞🏾 that this works.

@avinashparchuri avinashparchuri merged commit 75ce137 into neeva/v2.0 Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants