Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

backup: store backupmeta to s3#103

Merged
overvenus merged 14 commits intopingcap:masterfrom
DanielZhangQD:ms3
Dec 13, 2019
Merged

backup: store backupmeta to s3#103
overvenus merged 14 commits intopingcap:masterfrom
DanielZhangQD:ms3

Conversation

@DanielZhangQD
Copy link
Contributor

@DanielZhangQD DanielZhangQD commented Dec 11, 2019

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

overvenus
overvenus reviewed Dec 11, 2019
View reviewed changes
kennytm
kennytm reviewed Dec 11, 2019
View reviewed changes
@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

DanielZhangQD commented Dec 11, 2019

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

@DanielZhangQD
Copy link
Contributor Author

DanielZhangQD commented Dec 12, 2019

@kennytm @overvenus @tennix comments addressed, PTAL.

SunRunAway
SunRunAway reviewed Dec 12, 2019
View reviewed changes
kennytm
kennytm reviewed Dec 12, 2019
View reviewed changes
@SunRunAway SunRunAway mentioned this pull request Dec 12, 2019
Open
@DanielZhangQD
Copy link
Contributor Author

DanielZhangQD commented Dec 12, 2019

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

tennix
tennix reviewed Dec 12, 2019
View reviewed changes
@DanielZhangQD
Copy link
Contributor Author

DanielZhangQD commented Dec 12, 2019

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

gregwebs
gregwebs reviewed Dec 12, 2019
View reviewed changes
@gregwebs
Copy link
Contributor

gregwebs commented Dec 13, 2019

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

@kennytm kennytm Dec 13, 2019

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.

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 reviewed Dec 13, 2019
View reviewed changes
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
@DanielZhangQD DanielZhangQD deleted the ms3 branch December 13, 2019 07:30
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more 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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/LGT2 LGTM2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@DanielZhangQD @gregwebs @kennytm @SunRunAway @overvenus @tennix