PR feedback

Signed-off-by: Renan Rangel <[email protected]>
vitessio · Dec 12, 2024 · 9ab638a · 9ab638a
1 parent c4a5988
commit 9ab638a
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 18 deletions.
diff --git a/go/flags/endtoend/vtbackup.txt b/go/flags/endtoend/vtbackup.txt
@@ -195,7 +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)
+      --s3_backup_aws_min_partsize int                              Minimum part size to use, defaults to 5MiB but can be increased due to the dataset size. (default 5242880)
       --s3_backup_aws_region string                                 AWS region to use. (default "us-east-1")
       --s3_backup_aws_retries int                                   AWS request retries. (default -1)
       --s3_backup_force_path_style                                  force the s3 path style.

diff --git a/go/flags/endtoend/vtctld.txt b/go/flags/endtoend/vtctld.txt
@@ -110,7 +110,7 @@ Flags:
       --purge_logs_interval duration                                     how often try to remove old logs (default 1h0m0s)
       --remote_operation_timeout duration                                time to wait for a remote operation (default 15s)
       --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)
+      --s3_backup_aws_min_partsize int                                   Minimum part size to use, defaults to 5MiB but can be increased due to the dataset size. (default 5242880)
       --s3_backup_aws_region string                                      AWS region to use. (default "us-east-1")
       --s3_backup_aws_retries int                                        AWS request retries. (default -1)
       --s3_backup_force_path_style                                       force the s3 path style.

diff --git a/go/flags/endtoend/vttablet.txt b/go/flags/endtoend/vttablet.txt
@@ -312,7 +312,7 @@ Flags:
       --restore_from_backup_ts string                                    (init restore parameter) if set, restore the latest backup taken at or before this timestamp. Example: '2021-04-29.133050'
       --retain_online_ddl_tables duration                                How long should vttablet keep an old migrated table before purging it (default 24h0m0s)
       --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)
+      --s3_backup_aws_min_partsize int                                   Minimum part size to use, defaults to 5MiB but can be increased due to the dataset size. (default 5242880)
       --s3_backup_aws_region string                                      AWS region to use. (default "us-east-1")
       --s3_backup_aws_retries int                                        AWS request retries. (default -1)
       --s3_backup_force_path_style                                       force the s3 path style.

diff --git a/go/vt/mysqlctl/s3backupstorage/s3.go b/go/vt/mysqlctl/s3backupstorage/s3.go
@@ -58,6 +58,11 @@ import (
 	"vitess.io/vitess/go/vt/servenv"
 )
 
+const (
+	sseCustomerPrefix = "sse_c:"
+	MaxPartSize       = 1024 * 1024 * 1024 * 5 // 5GiB - limited by AWS https://docs.aws.amazon.com/AmazonS3/latest/userguide/qfacts.html
+)
+
 var (
 	// AWS API region
 	region string
@@ -89,7 +94,7 @@ var (
 	delimiter = "/"
 
 	// minimum part size
-	minimumPartSize int64
+	minPartSize int64
 
 	ErrPartSize = errors.New("minimum S3 part size must be between 5MiB and 5GiB")
 )
@@ -104,7 +109,7 @@ func registerFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&tlsSkipVerifyCert, "s3_backup_tls_skip_verify_cert", false, "skip the 'certificate is valid' check for SSL connections.")
 	fs.StringVar(&requiredLogLevel, "s3_backup_log_level", "LogOff", "determine the S3 loglevel to use from LogOff, LogDebug, LogDebugWithSigning, LogDebugWithHTTPBody, LogDebugWithRequestRetries, LogDebugWithRequestErrors.")
 	fs.StringVar(&sse, "s3_backup_server_side_encryption", "", "server-side encryption algorithm (e.g., AES256, aws:kms, sse_c:/path/to/key/file).")
-	fs.Int64Var(&minimumPartSize, "s3_backup_aws_minimum_partsize", 1024*1024*5, "Minimum part size to use")
+	fs.Int64Var(&minPartSize, "s3_backup_aws_min_partsize", manager.MinUploadPartSize, "Minimum part size to use, defaults to 5MiB but can be increased due to the dataset size.")
 }
 
 func init() {
@@ -118,8 +123,6 @@ type logNameToLogLevel map[string]aws.ClientLogMode
 
 var logNameMap logNameToLogLevel
 
-const sseCustomerPrefix = "sse_c:"
-
 type endpointResolver struct {
 	r        s3.EndpointResolverV2
 	endpoint *string
@@ -239,13 +242,13 @@ func calculateUploadPartSize(filesize int64) (partSizeBytes int64, err error) {
 		}
 	}
 
-	if minimumPartSize != 0 && partSizeBytes < minimumPartSize {
-		if minimumPartSize > 1024*1024*1024*5 || minimumPartSize < 1024*1024*5 { // 5GiB and 5MiB respectively
+	if minPartSize != 0 && partSizeBytes < minPartSize {
+		if minPartSize > 1024*1024*1024*5 || minPartSize < 1024*1024*5 { // 5GiB and 5MiB respectively
 			return 0, fmt.Errorf("%w, currently set to %s",
-				ErrPartSize, humanize.IBytes(uint64(minimumPartSize)),
+				ErrPartSize, humanize.IBytes(uint64(minPartSize)),
 			)
 		}
-		partSizeBytes = int64(minimumPartSize)
+		partSizeBytes = int64(minPartSize)
 	}
 
 	return

diff --git a/go/vt/mysqlctl/s3backupstorage/s3_mock.go b/go/vt/mysqlctl/s3backupstorage/s3_mock.go
@@ -162,7 +162,11 @@ func FailFirstWrite(s3bh *S3BackupHandle, ctx context.Context, filename string,
 		return nil, fmt.Errorf("AddFile cannot be called on read-only backup")
 	}
 
-	partSizeBytes := calculateUploadPartSize(filesize)
+	partSizeBytes, err := calculateUploadPartSize(filesize)
+	if err != nil {
+		return nil, err
+	}
+
 	reader, writer := io.Pipe()
 	r := io.Reader(reader)
 
@@ -181,7 +185,11 @@ func FailAllWrites(s3bh *S3BackupHandle, ctx context.Context, filename string, f
 		return nil, fmt.Errorf("AddFile cannot be called on read-only backup")
 	}
 
-	partSizeBytes := calculateUploadPartSize(filesize)
+	partSizeBytes, err := calculateUploadPartSize(filesize)
+	if err != nil {
+		return nil, err
+	}
+
 	reader, writer := io.Pipe()
 	r := &failReadPipeReader{PipeReader: reader}
 

diff --git a/go/vt/mysqlctl/s3backupstorage/s3_test.go b/go/vt/mysqlctl/s3backupstorage/s3_test.go
@@ -329,9 +329,9 @@ func TestWithParams(t *testing.T) {
 	assert.NotNil(t, s3.transport.Proxy)
 }
 
-func TestGetPartSize(t *testing.T) {
-	originalMinimum := minimumPartSize
-	defer func() { minimumPartSize = originalMinimum }()
+func TestCalculateUploadPartSize(t *testing.T) {
+	originalMinimum := minPartSize
+	defer func() { minPartSize = originalMinimum }()
 
 	tests := []struct {
 		name            string
@@ -386,8 +386,8 @@ func TestGetPartSize(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			minimumPartSize = tt.minimumPartSize
-			partSize, err := getPartSize(tt.filesize)
+			minPartSize = tt.minimumPartSize
+			partSize, err := calculateUploadPartSize(tt.filesize)
 			require.ErrorIs(t, err, tt.err)
 			require.Equal(t, tt.want, partSize)
 		})