Skip to content

*: use kvproto::backup::StorageBackend in create_storage (#6186)#6207

Merged
overvenus merged 4 commits intotikv:release-3.1from
kennytm:cherry-pick-6186-to-3.1
Dec 10, 2019
Merged

*: use kvproto::backup::StorageBackend in create_storage (#6186)#6207
overvenus merged 4 commits intotikv:release-3.1from
kennytm:cherry-pick-6186-to-3.1

Conversation

@kennytm
Copy link
Contributor

@kennytm kennytm commented Dec 10, 2019

What have you changed?

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

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/br#88

Benchmark result if necessary (optional)

Any examples? (optional)

@kennytm kennytm force-pushed the cherry-pick-6186-to-3.1 branch from d36589d to 3f04236 Compare December 10, 2019 06:33
@kennytm kennytm requested review from 5kbpers and overvenus December 10, 2019 06:47
debug!("download start";
"meta" => ?meta,
"url" => url,
"url" => ?backend,
Copy link
Contributor

Choose a reason for hiding this comment

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

The backend may contain secret data. I'm not sure if it's a good idea to log it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tennix how about just log the URL s3://bucket/prefix/? (we need to forward-port the patch to master)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no url field, for debug purpose, I think all other parameters need to be logged. For the secret, I think we should print <hidden-secret> or whatever meaningful for non-empty secret, and empty for empty secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the struct is generated by protobuf/prost so I don't think we can control its debug output (unless we use a custom formatter)

Signed-off-by: kennytm <kennytm@gmail.com>
@kennytm
Copy link
Contributor Author

kennytm commented Dec 10, 2019

PTAL @tennix @overvenus

@overvenus
Copy link
Member

Unfortunately, this PR is blocked by #6181 . 😿

Copy link
Contributor

@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

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
Copy link
Member

/run-all-tests

@overvenus
Copy link
Member

/run-all-tests

@overvenus overvenus merged commit 75eb28f into tikv:release-3.1 Dec 10, 2019
@kennytm kennytm deleted the cherry-pick-6186-to-3.1 branch December 10, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants