Skip to content

*: use kvproto::backup::StorageBackend in create_storage#6186

Merged
sre-bot merged 5 commits intotikv:masterfrom
kennytm:update-kvproto
Dec 9, 2019
Merged

*: use kvproto::backup::StorageBackend in create_storage#6186
sre-bot merged 5 commits intotikv:masterfrom
kennytm:update-kvproto

Conversation

@kennytm
Copy link
Contributor

@kennytm kennytm commented Dec 7, 2019

What have you changed?

Please explain in detail what the changes are in this PR and why they are needed:

  • In backup: extend backup storage for S3 pingcap/kvproto#507, we have removed the string-based "path" and "url" field from backup::BackupRequest and import_sstpb::DownloadRequest respectively, and replaced them with a structural type backup::StorageBackend, so that storage with multiple options (e.g. S3) can be passed without unnecessary deserialization.
  • This PR updates kvproto to include that change.
  • The external_storage::create_storage function is updated to take a &backup::StorageBackend instead of &str
  • The backup and sst_importer components and test cases are updated accordingly.

What is the type of the changes?

Pick one of the following and delete the others:

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How is the PR tested?

Please select the tests that you ran to verify your changes:

  • Unit test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect tidb-ansible?

No

Refer to a related PR or issue link (optional)

pingcap/kvproto#507
pingcap/br#88

Benchmark result if necessary (optional)

Any examples? (optional)

@kennytm kennytm added component/backup-restore Component: backup, import, external_storage needs-cherry-pick-release-3.1 Type: Need cherry pick to release 3.1 labels Dec 7, 2019
@kennytm kennytm force-pushed the update-kvproto branch 6 times, most recently from 549c67d to 86bd104 Compare December 8, 2019 09:39
Signed-off-by: kennytm <kennytm@gmail.com>
@kennytm
Copy link
Contributor Author

kennytm commented Dec 8, 2019

/run-all-tests

@kennytm
Copy link
Contributor Author

kennytm commented Dec 8, 2019

@kennytm kennytm requested review from 5kbpers and overvenus December 8, 2019 11:13
Copy link

@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

u
}

/// Creates a local `StorageBackend` to the given path.

Choose a reason for hiding this comment

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

S3 backend is not covered in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that would be another PR.

5kbpers
5kbpers previously approved these changes Dec 9, 2019
Copy link
Member

@5kbpers 5kbpers 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 Indicates that a PR has LGTM 1. label Dec 9, 2019
Copy link
Member

@5kbpers 5kbpers left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@overvenus overvenus added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

@kennytm merge failed.

@overvenus
Copy link
Member

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

@kennytm merge failed.

@kennytm
Copy link
Contributor Author

kennytm commented Dec 9, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

@kennytm merge failed.

@kennytm
Copy link
Contributor Author

kennytm commented Dec 9, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot sre-bot merged commit aead29b into tikv:master Dec 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

cherry pick to release-3.1 failed

kennytm added a commit to kennytm/tikv that referenced this pull request Dec 10, 2019
@kennytm kennytm deleted the update-kvproto branch December 10, 2019 06:47
overvenus pushed a commit that referenced this pull request Dec 10, 2019
* *: use kvproto::backup::StorageBackend in create_storage (#6186)

Signed-off-by: kennytm <kennytm@gmail.com>

* import: log url of backend instead of the entire backend itself

Signed-off-by: kennytm <kennytm@gmail.com>
c1ay pushed a commit to c1ay/tikv that referenced this pull request May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/backup-restore Component: backup, import, external_storage needs-cherry-pick-release-3.1 Type: Need cherry pick to release 3.1 status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants