fix(enterprise): Set version correctly post marshalling during restore #7018
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
The marshalPostingList function returns a KV without the version set and this causes issues if you restore a backup on top of an existing data. This PR fixes this issue by explicitly setting the version for the KV returned by the MarshalPostingList function.
rahulgurnani
requested review from
martinmr and
vvbalaji-dgraph
as code owners
|
Deploy preview for dgraph-docs ready! Built with commit 6232050 |
rahulgurnani
force-pushed
the
rahulgurnani/backup_restore
branch
from
3799931
to
4172438
Compare
| ctx := context.Background() | ||
|
|
||
| require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
Please confirm that these sleeps are good enough by running the tests multiple times. Otherwise these can be flaky.
There was a problem hiding this comment.
Good point. Ran it 10 times locally.
added 2 commits
November 30, 2020 20:53
| @@ -330,7 +399,7 @@ func runBackupInternal(t *testing.T, forceFull bool, numExpectedFiles, | |||
| copyToLocalFs(t) | |||
|
|
|||
| files := x.WalkPathFunc(copyBackupDir, func(path string, isdir bool) bool { | |||
| return !isdir && strings.HasSuffix(path, ".backup") | |||
| return !isdir && strings.HasSuffix(path, ".backup") && strings.HasPrefix(path, "data/backups_copy/dgraph.") | |||
There was a problem hiding this comment.
Use copyBackupDir instead.
There was a problem hiding this comment.
it would not help because copyBackupDir = ./data/backups_copy
| @@ -340,7 +409,7 @@ func runBackupInternal(t *testing.T, forceFull bool, numExpectedFiles, | |||
| require.Equal(t, numExpectedDirs, len(dirs)) | |||
|
|
|||
| manifests := x.WalkPathFunc(copyBackupDir, func(path string, isdir bool) bool { | |||
| return !isdir && strings.Contains(path, "manifest.json") | |||
| return !isdir && strings.Contains(path, "manifest.json") && strings.HasPrefix(path, "data/backups_copy/dgraph.") | |||
There was a problem hiding this comment.
Use copyBackupDir instead.
| copyBackupDir = "./data/backups_copy" | ||
| restoreDir = "./data/restore" | ||
| testDirs = []string{restoreDir, exporterDir} | ||
| exporterDir = "data/exporter" |
There was a problem hiding this comment.
I don't see exporterDir being used anywhere. Why do we have it?
There was a problem hiding this comment.
Thanks, cleaned it up
| func copyOldBackupDir(t *testing.T) { | ||
| destPath := testutil.DockerPrefix + "_alpha1_1:/data" | ||
| srchPath := "." + oldBackupDir | ||
| if err := testutil.DockerCp(srchPath, destPath); err != nil { |
There was a problem hiding this comment.
require.NoError please
rahulgurnani
force-pushed
the
rahulgurnani/backup_restore
branch
5 times, most recently
from
615e3e3
to
88a8323
Compare
jarifibrahim
approved these changes
Dec 2, 2020
rahulgurnani
force-pushed
the
rahulgurnani/backup_restore
branch
from
88a8323
to
6232050
Compare
rahulgurnani
added a commit
that referenced
this pull request
Dec 2, 2020
#7018) * Add test for old restore to a cluster * Fix author query * fix(restore): Use correct version after marshalling The marshalPostingList function returns a KV without the version set and this causes issues if you restore a backup on top of an existing data. This PR fixes this issue by explicitly setting the version for the KV returned by the MarshalPostingList function. * Fix test for restoring old backup incremental * Address review comments * Add old backup dir * Add wait for restore, address review comments * Make test use tls, refactor out waitforrestore Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
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.
Fixes DGRAPH-2775
The marshalPostingList function returns a KV without the version set and
this causes issues if you restore a backup on top of an existing data.
This PR fixes this issue by explicitly setting the version for the KV
returned by the MarshalPostingList function.
Also added a test which for incremental backup which was failing before this change.
This change is