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

*: extract storage package, and update kvproto#88

Merged
overvenus merged 11 commits intomasterfrom
kennytm/update-kvproto
Dec 11, 2019
Merged

*: extract storage package, and update kvproto#88
overvenus merged 11 commits intomasterfrom
kennytm/update-kvproto

Conversation

@kennytm
Copy link
Collaborator

@kennytm kennytm commented Dec 7, 2019

  1. Update kvproto which replaces the storage "path" field by a structured "StorageBackend" type.
  2. Replaces all use of such "path" by "StorageBackend"
  3. Extracted the storage module from pkg/util into pkg/storage. In the future I'd like to further move them into pingcap/tidb-tools so it could be reused by Lightning and Dumpling.
  4. Added some simple S3 awareness (not compatible with 【DNM】* *: support meta with s3 #66).

Closes #71.

@kennytm kennytm added the WIP label Dec 7, 2019
@kennytm kennytm force-pushed the kennytm/update-kvproto branch from 44b1b9e to 4ea349e Compare December 7, 2019 19:25
// FormatBackendURL obtains the raw URL which can be used the reconstruct the
// backend. The returned URL does not contain options for further configurating
// the backend. This is to avoid exposing secret tokens.
func FormatBackendURL(backend *backup.StorageBackend) (u url.URL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's only used for log now in the SaveBackMeta.

// S3EndpointOption is a BackendOption for changing the endpoint of the storage.
type S3Endpoint struct {
backendOption
// Endpoint is the host name of the S3 endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the format of http[s]://host[:port], right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. But Go's AWS SDK accepts just host:port too (and they use another flag to determine whether to use HTTPS).

@kennytm kennytm changed the title [DNM] *: extract storage package, and update kvproto *: extract storage package, and update kvproto Dec 10, 2019
@kennytm kennytm removed the WIP label Dec 10, 2019
@kennytm kennytm force-pushed the kennytm/update-kvproto branch from 96fbeef to 6acc073 Compare December 10, 2019 06:46
Copy link
Contributor

@DanielZhangQD DanielZhangQD 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 the status/LGT1 LGTM1 label Dec 10, 2019
@overvenus
Copy link
Member

Linter reports some suggestions, could you address them?

@kennytm kennytm force-pushed the kennytm/update-kvproto branch 2 times, most recently from 6dc17a2 to bb3a96b Compare December 10, 2019 10:46
@kennytm kennytm force-pushed the kennytm/update-kvproto branch from bb3a96b to 100dfde Compare December 10, 2019 10:50
@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #88   +/-   ##
=======================================
  Coverage   66.75%   66.75%           
=======================================
  Files          30       30           
  Lines        2620     2620           
=======================================
  Hits         1749     1749           
  Misses        629      629           
  Partials      242      242

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 89c1996...ecd45c6. Read the comment docs.

Copy link
Member

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

@overvenus
Copy link
Member

/run-integration-tests

Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus merged commit a7778a5 into master Dec 11, 2019
@overvenus overvenus deleted the kennytm/update-kvproto branch December 11, 2019 02:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The whole argument of --storage is stored in backupmeta which may include sensitive data

3 participants