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

Add GraphQL admin endpoint to list backups. #5307

Merged
merged 8 commits into from
Apr 30, 2020
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Apr 27, 2020

An endpoint to read the backup manifests in the given location.

Fixes DGRAPH-1229


This change is Reviewable

Docs Preview: Dgraph Preview

@martinmr martinmr requested review from manishrjain and a team as code owners April 27, 2020 21:56
@martinmr
Copy link
Contributor Author

@pawanrawal For some reason I cannot get partial queries to work. Even if I query for only one field, I am getting the entire response back. I suspect it's because I am returning a list instead of a map but I am not sure. The schema seems to work fine and the graphql recognizes all the fields. Somehow, they are not being used and I end up with all the data.

Could you look at the code in graphql/admin and see if I am doing something wrong? Thanks.

@pawanrawal
Copy link
Contributor

There is a bug which was introduced recently in 17a9c79. I have added a fix for it as part of #5317 which should be merged soon.

Copy link
Contributor

@manishrjain manishrjain 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, 1 unresolved discussion (waiting on @manishrjain, @martinmr, and @pawanrawal)


worker/backup_handler.go, line 1 at r2 (raw file):

// +build !oss

Can't do this.

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, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)


worker/backup_handler.go, line 1 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can't do this.

Done.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 10 files at r2, 4 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr and @pawanrawal)


graphql/admin/list_backups.go, line 1 at r3 (raw file):

package admin

License?

You'd need !oss flag?

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: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)


graphql/admin/list_backups.go, line 1 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

License?

You'd need !oss flag?

Done.

The flag is not needed here. The code in the worker package already has that flag. Doing this in the oss version will return an error saying the feature is not supported. We do the same with the backup endpoint.

@martinmr martinmr merged commit 56846ae into master Apr 30, 2020
@martinmr martinmr deleted the martinmr/list-backups branch April 30, 2020 18:41
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
An endpoint to read the backup manifests in the given location.

Fixes DGRAPH-1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants