-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Feat(Backup): Add native support for backup to Azure. #7843
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
Conversation
There was a problem hiding this 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 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @rohanprasad, and @vvbalaji-dgraph)
x/handlers.go, line 100 at r1 (raw file):
} if strings.HasSuffix(uri.Host, "blob.core.windows.net") {
Is this the right way? Why are we not using uri.Scheme
?
x/handlers.go, line 371 at r1 (raw file):
// Helper function to get the account, container and path of the destination folder from an Azure // URL. func getAzDetailsFromUri(uri *url.URL) (accountName, bucketName, pathName string, err error) {
Don't use named return values. They are confusing. Also avoid having more than 2 return values.
You can pack them in a struct.
x/handlers.go, line 375 at r1 (raw file):
parts := strings.Split(uri.Host, ".") if len(parts) > 0 { accountName = parts[0]
Wouldn't parts[0]
be https://<account_name>
. Do we want the https
part as well?
x/handlers.go, line 381 at r1 (raw file):
} parts = strings.Split(uri.Path, string(filepath.Separator))
Assuming path is https://<account_name>...
. Wouldn't parts[1] contain an empty string?
x/handlers.go, line 100 at r1 (raw file): Previously, ahsanbarkati (Ahsan Barkati) wrote…
Haven't found a scheme-based URL for Azure blob storage. Assuming an https path here: |
x/handlers.go, line 371 at r1 (raw file): Previously, ahsanbarkati (Ahsan Barkati) wrote…
Will move them in a struct. |
x/handlers.go, line 375 at r1 (raw file): Previously, ahsanbarkati (Ahsan Barkati) wrote…
Host doesn't contains scheme, so this should work well. |
x/handlers.go, line 381 at r1 (raw file): Previously, ahsanbarkati (Ahsan Barkati) wrote…
Path is of the form: /container/path/to/object. When split, parts[0] will be empty and parts[1] will contain the container/bucket name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please test these APIs manually.
Fixes DGRAPH-3384
Currently, we use MinIO to write backup files to Azure blob storage. To do this we need to start a minio server (with Azure credentials) and trigger a backup with the destination as the minio server. This requires the user to run the minio server every time (or always) when a backup needs to be triggered. Using native SDK will help get rid of the extra service.
Azure storage (AZS) requires the user to get secret. When starting an alpha we can set env variable:
AZURE_STORAGE_KEY="secret_key_here" dgraph alpha
to allow access to AZS. To trigger a backup the user can use the admin endpoint to initiate the backup. The destination should be set as:
"https://<account_name>.blob.core.windows.net//path/to/dest"
This change is