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.

Sign up for GitHub

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

Jump to bottom

backup: store backupmeta to s3 #103

Merged
merged 14 commits into from
Dec 13, 2019
Merged

backup: store backupmeta to s3 #103

merged 14 commits into from
Dec 13, 2019

Conversation

DanielZhangQD
Copy link
Contributor

Support storing backup meta to s3, and support backup and restore with s3.

overvenus
overvenus reviewed Dec 11, 2019
View reviewed changes
pkg/storage/flags.go Show resolved Hide resolved
pkg/storage/parse.go Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
kennytm
kennytm reviewed Dec 11, 2019
View reviewed changes
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/storage.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 11, 2019 •
edited
Loading

Codecov Report

Merging #103 into master will increase coverage by 0.61%.
The diff coverage is 82.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   69.97%   70.59%   +0.61%     
==========================================
  Files          31       31              
  Lines        2911     3050     +139     
==========================================
+ Hits         2037     2153     +116     
- Misses        582      596      +14     
- Partials      292      301       +9
Impacted Files Coverage Δ
pkg/storage/parse.go 87.75% <100%> (+0.79%) ⬆️
pkg/storage/flags.go 57.89% <100%> (+4.95%) ⬆️
pkg/backup/client.go 71.14% <25%> (+0.06%) ⬆️
pkg/storage/storage.go 42.85% <50%> (+2.85%) ⬆️
pkg/storage/s3.go 81.87% <84.44%> (+11.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d11c5c...115aa7c. Read the comment docs.

@DanielZhangQD
Copy link
Contributor Author

@kennytm @overvenus @tennix PTAL new commit with aws-sdk.

@DanielZhangQD
Copy link
Contributor Author

@kennytm @overvenus @tennix comments addressed, PTAL.

SunRunAway
SunRunAway reviewed Dec 12, 2019
View reviewed changes
pkg/storage/s3.go Outdated Show resolved Hide resolved
kennytm
kennytm reviewed Dec 12, 2019
View reviewed changes
pkg/backup/client.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
@SunRunAway SunRunAway mentioned this pull request Dec 12, 2019
Open
@DanielZhangQD
Copy link
Contributor Author

@kennytm @overvenus @SunRunAway comments addressed, PTAL again.

tennix
tennix reviewed Dec 12, 2019
View reviewed changes
pkg/storage/flags.go Outdated Show resolved Hide resolved
pkg/storage/parse_test.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
@DanielZhangQD
Copy link
Contributor Author

@kennytm @SunRunAway @overvenus @tennix comments addressed, PTAL again.

gregwebs
gregwebs reviewed Dec 12, 2019
View reviewed changes
pkg/storage/s3.go Outdated Show resolved Hide resolved
@gregwebs
Copy link
Contributor

LGTM

@kennytm kennytm added the status/LGT1 LGTM1 label Dec 13, 2019
kennytm
kennytm reviewed Dec 13, 2019
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Rest LGTM

pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
pkg/storage/s3.go
flags.String(s3StorageClassOption, "", "Set the S3 storage class, e.g. STANDARD")
flags.String(s3SSEOption, "", "Set the S3 server-side encryption algorithm, e.g. AES256")
flags.String(s3ACLOption, "", "Set the S3 canned ACLs, e.g. authenticated-read")
flags.String(s3ProviderOption, "", "Set the S3 provider, e.g. aws, alibaba, ceph")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This setting doesn't seem to have any effect yet besides force-path-style. I suggest exposing force-path-style instead of this option, until the provider can really change the endpoint etc.

pkg/storage/s3_test.go Outdated Show resolved Hide resolved
pkg/storage/s3.go Show resolved Hide resolved
pkg/storage/s3.go Outdated Show resolved Hide resolved
kennytm
kennytm approved these changes Dec 13, 2019
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT2 LGTM2 and removed status/LGT1 LGTM1 labels Dec 13, 2019
tennix
tennix approved these changes Dec 13, 2019
View reviewed changes
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@overvenus overvenus merged commit bfe4e23 into pingcap:master Dec 13, 2019
@tennix tennix mentioned this pull request Dec 16, 2019
Open
9 tasks
This was referenced Dec 17, 2019
Merged
Closed
overvenus pushed a commit to overvenus/br-1 that referenced this pull request Dec 29, 2020
@kennytm
parser: allow one level of nesting when parsing rows (pingcap#103)
2428999
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregwebs gregwebs gregwebs left review comments

@SunRunAway SunRunAway SunRunAway left review comments

@overvenus overvenus overvenus left review comments

@kennytm kennytm kennytm approved these changes

@tennix tennix tennix approved these changes

Assignees
No one assigned
Labels
status/LGT2 LGTM2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@DanielZhangQD @gregwebs @kennytm @SunRunAway @overvenus @tennix