Fix manifest corruption #1756
Merged
Fix manifest corruption #1756
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
|
|
…ncy on os.file.Sync()
…y on os.file.Sync()
iluminae
force-pushed
the
fix_manifest_corruption
branch
from
4f2f2cf
to
bf9578f
Compare
jarifibrahim
approved these changes
Nov 7, 2021
NamanJain8
reviewed
Nov 8, 2021
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @iluminae and @manishrjain)
a discussion (no related file):
Awesome @iluminae, this is a nice catch.
Thanks @jarifibrahim for the review. :)
NamanJain8
approved these changes
Nov 8, 2021
|
fyi @billprovince @joshua-goldstein - +1 from me to get this in |
joshua-goldstein
pushed a commit
that referenced
this pull request
Oct 18, 2022
…rrent compactions (#1756) `manifestFile.rewrite` closes the file due to which `manifest.fp.Sync()` can fail. This leads to an updated manifest but non-updated tables set. Hence, the compaction process enters into an infinite loop failing with the `MANIFEST removes non-existing table` error. This PR fixes it. (cherry picked from commit 6ed45ae)
joshua-goldstein
pushed a commit
that referenced
this pull request
Nov 3, 2022
…rrent compactions (#1756) `manifestFile.rewrite` closes the file due to which `manifest.fp.Sync()` can fail. This leads to an updated manifest but non-updated tables set. Hence, the compaction process enters into an infinite loop failing with the `MANIFEST removes non-existing table` error. This PR fixes it. (cherry picked from commit 6ed45ae)
joshua-goldstein
pushed a commit
that referenced
this pull request
Nov 3, 2022
…rrent compactions (#1756) `manifestFile.rewrite` closes the file due to which `manifest.fp.Sync()` can fail. This leads to an updated manifest but non-updated tables set. Hence, the compaction process enters into an infinite loop failing with the `MANIFEST removes non-existing table` error. This PR fixes it. (cherry picked from commit 6ed45ae)
skrdgraph
pushed a commit
that referenced
this pull request
Nov 3, 2022
…rrent compactions (#1756) (#1820) This PR is a cherry pick of #1756. Addresses [this issue](https://discuss.dgraph.io/t/badgerdb-manifest-corruption-issue-solution/15915) on Discuss and[ this issue](#1819) on Badger. Co-authored-by: Kenan Kessler <kesken88@gmail.com>
mYmNeo
added a commit
to mYmNeo/badger
that referenced
this pull request
Feb 10, 2023
mYmNeo
added a commit
to mYmNeo/badger
that referenced
this pull request
Feb 13, 2023
mangalaman93
pushed a commit
that referenced
this pull request
Feb 14, 2023
…rrent compactions (#1756) (#1820) This PR is a cherry pick of #1756. Addresses [this issue](https://discuss.dgraph.io/t/badgerdb-manifest-corruption-issue-solution/15915) on Discuss and[ this issue](#1819) on Badger. Co-authored-by: Kenan Kessler <kesken88@gmail.com>
skrdgraph
pushed a commit
to dgraph-io/dgraph
that referenced
this pull request
Mar 7, 2023
Update Badger from `7677fcb` to `6ed45ae` (on main-deprecated branch). This fixes dgraph-io/badger#1752 and dgraph-io/badger#1756. First CI failed [here](https://github.com/dgraph-io/dgraph/actions/runs/4208376321/jobs/7304322951). This issue was resolved by bumping up the Go version used in the container.
fredcarle
pushed a commit
to fredcarle/badger
that referenced
this pull request
Aug 1, 2023
…rrent compactions (dgraph-io#1756) `manifestFile.rewrite` closes the file due to which `manifest.fp.Sync()` can fail. This leads to an updated manifest but non-updated tables set. Hence, the compaction process enters into an infinite loop failing with the `MANIFEST removes non-existing table` error. This PR fixes it.
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.
This fixes the manifest corruption posted about here
I was able to reproduce the race condition every time by injecting a sleep into the sync call during tests only. I am not sure if you guys would rather have this or a test that is much longer and may or may not reproduce based on high concurrency (but does not inject the sleep) or no test at all.
Let me know - I made it so we can just revert the test commit and it should be good to go.
This change is