-
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 timeout for mysqld_shutdown #6849
Add timeout for mysqld_shutdown #6849
Conversation
…down before aborting Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…sses Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@@ -182,7 +187,9 @@ func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupP | |||
params.Logger.Infof("using replication position: %v", replicationPosition) | |||
|
|||
// shutdown mysqld | |||
err = params.Mysqld.Shutdown(ctx, params.Cnf, true) | |||
shutdownCtx, cancel := context.WithTimeout(ctx, *builtinBackupMysqldDeadline) | |||
err = params.Mysqld.Shutdown(shutdownCtx, params.Cnf, true) |
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 don't see an easy way to test this. I can't use fakemysqldaemon
to create a mysqld that hangs on shutdown, because that package imports this package, which causes circular import errors. 🤔
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.
Solved this with blackbox testing ✅
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…behavior Signed-off-by: Andrew Mason <amason@slack-corp.com>
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.
Nice work! Just the one comment.
@@ -47,6 +48,13 @@ const ( | |||
dataDictionaryFile = "mysql.ibd" | |||
) | |||
|
|||
var ( | |||
// BuiltinBackupMysqldDeadline is how long ExecuteBackup should wait for response from mysqld.Shutdown. |
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.
typically deadline
is used for timestamps and timeout
for durations. This looks like a timeout.
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188 would you please create another PR to merge this into release-8.0? |
…meout Add timeout for mysqld_shutdown Signed-off-by: Andrew Mason <amason@slack-corp.com>
Backport pull request #6849 (add timeout for mysqld_shutdown) to 8.0
Resolves #6848
Notable changes:
ExecuteContext
method. All hooks are now implemented withexec.CommandContext
under the hood instead ofexec.Command
. Current callers are unaffected, and can opt in to switching over at their discretion.HookResult.ExitStatus
type for hooks that exceed their context deadline/timeout.FakeMysqlDaemon.Start()
andFakeMysqlDaemon.Shutdown()
are configurable via theFakeMysqlDaemon.StartupTime
andFakeMysqlDaemon.ShutdownTime
respectively, to artificially cause delay in tests.