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(Backup): Add native support for backup to Azure. #7843

Merged
merged 3 commits into from May 25, 2021

Conversation

rohanprasad
Copy link
Contributor

@rohanprasad rohanprasad commented May 21, 2021

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 Reviewable

Copy link
Contributor

@ahsanbarkati ahsanbarkati 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 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?

@rohanprasad
Copy link
Contributor Author


x/handlers.go, line 100 at r1 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

Is this the right way? Why are we not using uri.Scheme?

Haven't found a scheme-based URL for Azure blob storage. Assuming an https path here:
https://<account_name>.blob.core.windows.net//path/to/object (added in comments as well)

@rohanprasad
Copy link
Contributor Author


x/handlers.go, line 371 at r1 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

Don't use named return values. They are confusing. Also avoid having more than 2 return values.
You can pack them in a struct.

Will move them in a struct.

@rohanprasad
Copy link
Contributor Author


x/handlers.go, line 375 at r1 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

Wouldn't parts[0] be https://<account_name>. Do we want the https part as well?

Host doesn't contains scheme, so this should work well.

@rohanprasad
Copy link
Contributor Author


x/handlers.go, line 381 at r1 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

Assuming path is https://<account_name>.... Wouldn't parts[1] contain an empty string?

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.

Copy link
Contributor

@ahsanbarkati ahsanbarkati left a 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.

x/handlers.go Outdated Show resolved Hide resolved
@rohanprasad rohanprasad merged commit 556d7fa into master May 25, 2021
@rohanprasad rohanprasad deleted the rohanprasad/add_azure_native branch May 25, 2021 18:37
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.

None yet

2 participants