-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add S3 minimum part size defined by the user #17171
base: main
Are you sure you want to change the base?
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17171 +/- ##
==========================================
+ Coverage 67.37% 67.39% +0.01%
==========================================
Files 1573 1573
Lines 253113 253130 +17
==========================================
+ Hits 170538 170592 +54
+ Misses 82575 82538 -37 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
44ede6d
to
807c22b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a couple of minor comments.
go/flags/endtoend/vtbackup.txt
Outdated
@@ -195,6 +195,7 @@ Flags: | |||
--remote_operation_timeout duration time to wait for a remote operation (default 15s) | |||
--restart_before_backup Perform a mysqld clean/full restart after applying binlogs, but before taking the backup. Only makes sense to work around xtrabackup bugs. | |||
--s3_backup_aws_endpoint string endpoint of the S3 backend (region must be provided). | |||
--s3_backup_aws_minimum_partsize int Minimum part size to use (default 5242880) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but we use min
in other existing flags so we can shorten this one as well. New flags should also use dashes rather than underscores as we want to transition to the latter for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this flag is part of a group... we may actually just want to keep them all uniform...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it to min
, but do let me know if the preferred is to use the dashes instead of underscores for it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would favor uniformity for this config group over moving to dashes, and move these all together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably spell out what the default is and add a period to be consistent with other flags in the same group. The default shown by the helper is not very intuitive at first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it :)
go/vt/mysqlctl/s3backupstorage/s3.go
Outdated
} | ||
|
||
if minimumPartSize != 0 && partSizeBytes < minimumPartSize { | ||
if minimumPartSize > 1024*1024*1024*5 || minimumPartSize < 1024*1024*5 { // 5GiB and 5MiB respectively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do these min and max values come from? I would hope that there are consts we can use instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a great point. for the minimum, the SDK does define it, so we could use manager.MinUploadPartSize
, but I don't see anything for the max. I will take a better look when I update the code for the flag change mentioned above, but we can at least change that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use the constant provided by the SDK for the minimum. For the maximum there is none, it's fine to hardcode it but it should be a constant for clarity with a link to AWS's docs: https://docs.aws.amazon.com/AmazonS3/latest/userguide/qfacts.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update it as well :)
…7194) Signed-off-by: Matt Lord <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
…x volume (vitessio#17118) Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
…itessio#17207) Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: GitHub <[email protected]> Signed-off-by: Florent Poinsard <[email protected]> Co-authored-by: frouioui <[email protected]> Co-authored-by: Florent Poinsard <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
…17220) Signed-off-by: Matt Lord <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
…itessio#17226) Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
…sio#17231) Signed-off-by: Gabriel Parreiras <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Derek Perkins <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
…. CUTOVER_THRESHOLD ...` command (vitessio#17126) Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Dirkjan Bussink <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Renan Rangel <[email protected]>
7f7d336
to
0f08ff4
Compare
Signed-off-by: 'Renan Rangel' <[email protected]>
Hey @rvrangel the history of that Pull Request looks a bit broken. It needs to be rebased. |
Description
This adds a new parameter to allow the operator to specify a minimum part size for the S3 uploads.
Related Issue(s)
fixes #17175
Checklist
Deployment Notes