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

feat(Dgraph): Online restores allows to restore a specific backup. #6411

Merged
merged 11 commits into from
Sep 14, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Sep 4, 2020

Currently online restores will restore the entire backup series. This PR allows to
restore the backup series up to a specific backup.

Fixes DGRAPH-1648


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Sep 4, 2020
@martinmr
Copy link
Contributor Author

martinmr commented Sep 4, 2020

Disregard the changes to the docker-compose files and the makefiles. They are a workaround to an issue I'm having (my workstation has a different version of glibc than the docker image). I will revert these changes before I merge this PR.

@martinmr martinmr requested a review from a team September 4, 2020 22:17
Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Took a stab. Lets do synchronous review so I can understand better.

protos/pb.proto Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)


systest/online-restore/online_restore_test.go, line 178 at r3 (raw file):

t.Logf(string(resp.GetJson()))
			t.Logf(string(bodies[1]))

revert this


worker/backup_handler.go, line 63 at r3 (raw file):

 GetManfiest

typo

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

:lgtm: minor comments. Feel free to resolve.

Reviewable status: 0 of 13 files reviewed, 5 unresolved discussions (waiting on @manishrjain, @martinmr, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)


dgraph/Makefile, line 46 at r3 (raw file):

lastCommitTime  = github.com/dgraph-io/dgraph/x.lastCommitTime

BUILD_FLAGS   ?= -ldflags '-X ${lastCommitSHA}=${BUILD} -X "${lastCommitTime}=${BUILD_DATE}" -X "${dgraphVersion}=${BUILD_VERSION}" -X "${dgraphCodename}=${BUILD_CODENAME}${MODIFIED}" -X ${gitBranch}=${BUILD_BRANCH}' -tags netgo

what is netgo tag for?


worker/backup_handler.go, line 66 at r3 (raw file):

	// and backup number at the specified location. If backupNum is set to zero,
	// all the manifests for the backup series will be returned.
	GetManifests(*url.URL, string, uint64) ([]*Manifest, error)

Maybe clarify comment that given a backupNum, it returns manifests from 0 to thru the backupNum. Not clear as it is.


worker/backup_handler.go, line 318 at r3 (raw file):

	if backupNum > 0 {
		if len(manifests) < int(backupNum) {

are we off by 1? e.g. if backupNum = 1, i expect 2 manifests at 0 and 1, no?

Copy link
Contributor Author

@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.

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @parasssh, @pawanrawal, and @vvbalaji-dgraph)


dgraph/Makefile, line 46 at r3 (raw file):

Previously, parasssh wrote…

what is netgo tag for?

Ignore that. It's just because I couldn't build Dgraph on my machine. I'll revert that.

But the tag is to build the net library without using the c standard library.


systest/online-restore/online_restore_test.go, line 178 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
t.Logf(string(resp.GetJson()))
			t.Logf(string(bodies[1]))

revert this

Done.


worker/backup_handler.go, line 63 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
 GetManfiest

typo

Done.


worker/backup_handler.go, line 66 at r3 (raw file):

Previously, parasssh wrote…

Maybe clarify comment that given a backupNum, it returns manifests from 0 to thru the backupNum. Not clear as it is.

Done.


worker/backup_handler.go, line 318 at r3 (raw file):

Previously, parasssh wrote…

are we off by 1? e.g. if backupNum = 1, i expect 2 manifests at 0 and 1, no?

There's no backup with backupNum 0. We start the count at 1.

@martinmr martinmr merged commit a459c75 into master Sep 14, 2020
@martinmr martinmr deleted the martinmr/restore-num branch September 14, 2020 20:48
akashjain971 pushed a commit that referenced this pull request Oct 16, 2020
…6411)

Currently online restores will restore the entire backup series. This PR allows to
restore the backup series up to a specific backup.

Fixes DGRAPH-1648
akashjain971 pushed a commit that referenced this pull request Oct 22, 2020
…6411)

Currently online restores will restore the entire backup series. This PR allows to
restore the backup series up to a specific backup.

Fixes DGRAPH-1648
akashjain971 added a commit that referenced this pull request Oct 23, 2020
…6411) (#6742)

Currently online restores will restore the entire backup series. This PR allows to
restore the backup series up to a specific backup.

Fixes DGRAPH-1648

Co-authored-by: Martin Martinez Rivera <martinmr@dgraph.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants