Allow for specifying a specific MySQL shutdown timeout #17923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds an override flag to be able to set the shutdown timeout for MySQL for an individual backup without having to override the global flag for a tablet. Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
added
Type: Enhancement
dbussink
requested review from
deepthi,
harshit-gangal,
mattlord,
notfelineit,
GuptaManan100,
rohit-nayak-ps and
shlomi-noach
as code owners
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
|
vitess-bot
bot
added
NeedsBackportReason
dbussink
removed
NeedsDescriptionUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17923 +/- ##
=======================================
Coverage 67.56% 67.56%
=======================================
Files 1597 1597
Lines 259775 259803 +28
=======================================
+ Hits 175509 175530 +21
- Misses 84266 84273 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
frouioui
reviewed
Mar 6, 2025
| Concurrency int32 | ||
| IncrementalFromPos string | ||
| UpgradeSafe bool | ||
| MysqlShutdownTimeout time.Duration |
There was a problem hiding this comment.
New CLI documentation will have to be generated for this subcommand: https://vitess.io/docs/22.0/reference/programs/vtctldclient/vtctldclient_backup/
This should be picked up when we re-generate the CLI docs before v22.0.0, but just noting it here.
| @@ -74,6 +74,7 @@ func commandBackup(ctx context.Context, wr *wrangler.Wrangler, subFlags *pflag.F | |||
| allowPrimary := subFlags.Bool("allow_primary", false, "Allows backups to be taken on primary. Warning!! If you are using the builtin backup engine, this will shutdown your primary mysql for as long as it takes to create a backup.") | |||
| incrementalFromPos := subFlags.String("incremental_from_pos", "", "Position, or name of backup from which to create an incremental backup. Default: empty. If given, then this backup becomes an incremental backup from given position or given backup. If value is 'auto', this backup will be taken from the last successful backup position.") | |||
| upgradeSafe := subFlags.Bool("upgrade-safe", false, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.") | |||
| mysqlShutdownTimeout := subFlags.Duration("mysql-shutdown-timeout", mysqlctl.DefaultShutdownTimeout, "timeout to use when MySQL is being shut down.") | |||
There was a problem hiding this comment.
Let's make the first letter upper case to match what we do in the other flags.
Suggested change
| mysqlShutdownTimeout := subFlags.Duration("mysql-shutdown-timeout", mysqlctl.DefaultShutdownTimeout, "timeout to use when MySQL is being shut down.") | |
| mysqlShutdownTimeout := subFlags.Duration("mysql-shutdown-timeout", mysqlctl.DefaultShutdownTimeout, "Timeout to use when MySQL is being shut down.") |
There was a problem hiding this comment.
I made these consistent with all the other spots where we have the same flag.
If we're changing it, we should also change it for all existing flags with the same name?
| Root.AddCommand(Backup) | ||
|
|
||
| BackupShard.Flags().BoolVar(&backupShardOptions.AllowPrimary, "allow-primary", false, "Allow the primary of a shard to be used for the backup. WARNING: If using the builtin backup engine, this will shutdown mysqld on the primary and stop writes for the duration of the backup.") | ||
| BackupShard.Flags().Int32Var(&backupShardOptions.Concurrency, "concurrency", 4, "Specifies the number of compression/checksum jobs to run simultaneously.") | ||
| BackupShard.Flags().StringVar(&backupShardOptions.IncrementalFromPos, "incremental-from-pos", "", "Position, or name of backup from which to create an incremental backup. Default: empty. If given, then this backup becomes an incremental backup from given position or given backup. If value is 'auto', this backup will be taken from the last successful backup position.") | ||
| BackupShard.Flags().BoolVar(&backupOptions.UpgradeSafe, "upgrade-safe", false, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.") | ||
| BackupShard.Flags().BoolVar(&backupShardOptions.UpgradeSafe, "upgrade-safe", false, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.") | ||
| BackupShard.Flags().DurationVar(&backupShardOptions.MysqlShutdownTimeout, "mysql-shutdown-timeout", mysqlctl.DefaultShutdownTimeout, "timeout to use when MySQL is being shut down.") |
There was a problem hiding this comment.
Same here, let's capitalize the first letter of the helper.
| @@ -294,12 +298,14 @@ func init() { | |||
| Backup.Flags().StringVar(&backupOptions.BackupEngine, "backup-engine", "", "Request a specific backup engine for this backup request. Defaults to the preferred backup engine of the target vttablet") | |||
|
|
|||
| Backup.Flags().BoolVar(&backupOptions.UpgradeSafe, "upgrade-safe", false, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.") | |||
| Backup.Flags().DurationVar(&backupOptions.MysqlShutdownTimeout, "mysql-shutdown-timeout", mysqlctl.DefaultShutdownTimeout, "timeout to use when MySQL is being shut down.") | |||
|
@frouioui we could add an end to end test, but it wouldn't really be to exercise the new option. Tests are really small scale setups here and to reproduce this kind of timeout problem specifically you need both a very large dataset and also modify it significantly before attempting the shutdown. If you're talking about just having something to validate it end to end, then sure, it does add some value for that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds an override flag to be able to set the shutdown timeout for MySQL for an individual backup without having to override the global flag for a tablet.
Related Issue(s)
Fixes #17920
Checklist