Skip to content

backup: extend backup storage for S3#507

Merged
kennytm merged 6 commits intopingcap:masterfrom
tennix:extend-backup-storage
Dec 5, 2019
Merged

backup: extend backup storage for S3#507
kennytm merged 6 commits intopingcap:masterfrom
tennix:extend-backup-storage

Conversation

@tennix
Copy link
Member

@tennix tennix commented Dec 4, 2019

Signed-off-by: tennix ztennix@gmail.com

Extend backup storage to use support extra storage backend to avoid encoding all parameters in the path field as URL format.

Note: I'll generate the code in another commit once the protobuf definition is approved to make the review easier.

@tennix tennix requested a review from overvenus December 4, 2019 06:22
@claassistantio
Copy link

claassistantio commented Dec 4, 2019

CLA assistant check
All committers have signed the CLA.

@tennix tennix requested a review from kennytm December 4, 2019 06:23
Signed-off-by: tennix <ztennix@gmail.com>
@tennix tennix force-pushed the extend-backup-storage branch from 5706edf to dc0e11d Compare December 4, 2019 06:29
@tennix tennix requested a review from DanielZhangQD December 4, 2019 06:59
Copy link
Contributor

@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 (the content of S3 may still need to be changed, but let's defer that for now)

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

Signed-off-by: tennix <ztennix@gmail.com>
@tennix tennix force-pushed the extend-backup-storage branch from 64c5987 to 4ce8f96 Compare December 4, 2019 13:15
@tennix tennix requested a review from kennytm December 4, 2019 13:17
Copy link
Contributor

@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. Please generate the Go code.

BTW maybe use an empty message for noop instead of a bool?

}
}

message S3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for provider?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can treat non-AWS providers as custom providers, and enfore to provide endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, by enforce, we can only make it clear in the document, right? We cannot enforce it in code without provider.

Signed-off-by: tennix <ztennix@gmail.com>
@tennix tennix force-pushed the extend-backup-storage branch from ac21b35 to 68ee98a Compare December 5, 2019 02:41
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

Signed-off-by: tennix <ztennix@gmail.com>
kennytm
kennytm previously approved these changes Dec 5, 2019
DanielZhangQD
DanielZhangQD previously approved these changes Dec 5, 2019
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

Signed-off-by: tennix <ztennix@gmail.com>
@tennix tennix dismissed stale reviews from DanielZhangQD and kennytm via aea2acf December 5, 2019 07:05
Signed-off-by: tennix <ztennix@gmail.com>
Copy link
Contributor

@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

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

@kennytm kennytm merged commit ee6b9c1 into pingcap:master Dec 5, 2019
@tennix tennix deleted the extend-backup-storage branch December 5, 2019 08:58
kennytm added a commit that referenced this pull request Dec 10, 2019
Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>
Co-authored-by: Tennix <tennix@users.noreply.github.com>
Signed-off-by: kennytm <kennytm@gmail.com>
kennytm added a commit that referenced this pull request Dec 10, 2019
Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>
Co-authored-by: Tennix <tennix@users.noreply.github.com>
Signed-off-by: kennytm <kennytm@gmail.com>
daimashusheng pushed a commit to daimashusheng/kvproto that referenced this pull request Sep 2, 2021
* extend backup storage for S3

Signed-off-by: tennix <ztennix@gmail.com>

* address comment

Signed-off-by: tennix <ztennix@gmail.com>

* use empty message for noop storage backend

Signed-off-by: tennix <ztennix@gmail.com>

* Generate Go code

Signed-off-by: tennix <ztennix@gmail.com>

* Deprecate local_path

Signed-off-by: tennix <ztennix@gmail.com>

* Make StorageBackend a standalone message type

Signed-off-by: tennix <ztennix@gmail.com>
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.

5 participants