From f89442a1d67ea8533145c69eb053d30ffa20a303 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Thu, 6 Apr 2023 14:53:08 -0700 Subject: [PATCH] fixing bug #12830 Signed-off-by: Rameez Sajwani --- go/vt/mysqlctl/builtinbackupengine.go | 8 ++++---- go/vt/mysqlctl/builtinbackupengine_test.go | 15 ++++++++++----- go/vt/mysqlctl/mysqld.go | 2 +- go/vt/mysqlctl/query.go | 4 ++-- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index c1fe10f199c..13a9d58cf77 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -527,7 +527,7 @@ func (be *BuiltinBackupEngine) backupFiles( // unpredictability in my test cases, so in order to avoid that, I am adding this cancellation check. select { case <-ctx.Done(): - log.Errorf("Context cancelled during %q backup", fe.Name) + log.Errorf("Context canceled during %q backup", fe.Name) bh.RecordError(vterrors.Errorf(vtrpc.Code_CANCELED, "context canceled")) return default: @@ -803,7 +803,7 @@ func (be *BuiltinBackupEngine) executeRestoreFullBackup(ctx context.Context, par params.Logger.Infof("Restore: copying %v files", len(bm.FileEntries)) - if _, err := be.restoreFiles(context.Background(), params, bh, bm); err != nil { + if _, err := be.restoreFiles(ctx, params, bh, bm); err != nil { // don't delete the file here because that is how we detect an interrupted restore return vterrors.Wrap(err, "failed to restore files") } @@ -816,7 +816,7 @@ func (be *BuiltinBackupEngine) executeRestoreFullBackup(ctx context.Context, par // The underlying mysql database is expected to be up and running. func (be *BuiltinBackupEngine) executeRestoreIncrementalBackup(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle, bm builtinBackupManifest) error { params.Logger.Infof("Restoring incremental backup to position: %v", bm.Position) - createdDir, err := be.restoreFiles(context.Background(), params, bh, bm) + createdDir, err := be.restoreFiles(ctx, params, bh, bm) defer os.RemoveAll(createdDir) mysqld, ok := params.Mysqld.(*Mysqld) if !ok { @@ -913,7 +913,7 @@ func (be *BuiltinBackupEngine) restoreFiles(ctx context.Context, params RestoreP // unpredictability in my test cases, so in order to avoid that, I am adding this cancellation check. select { case <-ctx.Done(): - log.Errorf("Context cancelled during %q backup", fe.Name) + log.Errorf("Context canceled during %q backup", fe.Name) rec.RecordError(vterrors.Errorf(vtrpc.Code_CANCELED, "context canceled")) return default: diff --git a/go/vt/mysqlctl/builtinbackupengine_test.go b/go/vt/mysqlctl/builtinbackupengine_test.go index dcba47b0eab..fe5428d2f1e 100644 --- a/go/vt/mysqlctl/builtinbackupengine_test.go +++ b/go/vt/mysqlctl/builtinbackupengine_test.go @@ -22,6 +22,7 @@ import ( "fmt" "os" "path" + "strings" "testing" "time" @@ -163,9 +164,9 @@ func TestExecuteBackup(t *testing.T) { assert.False(t, ok) } -// TestExecuteBackupWithCancelledContext tests the ability of the backup function to gracefully handle cases where errors +// TestExecuteBackupWithCanceledContext tests the ability of the backup function to gracefully handle cases where errors // occur due to various reasons, such as context time cancel. The process should not panic in these situations. -func TestExecuteBackupWithCancelledContext(t *testing.T) { +func TestExecuteBackupWithCanceledContext(t *testing.T) { // Set up local backup directory id := fmt.Sprintf("%d", time.Now().UnixNano()) backupRoot := fmt.Sprintf("testdata/builtinbackup_test_%s", id) @@ -245,7 +246,7 @@ func TestExecuteBackupWithCancelledContext(t *testing.T) { assert.False(t, ok) } -// TestExecuteRestoreWithCancelledContext tests the ability of the restore function to gracefully handle cases where errors +// TestExecuteRestoreWithCanceledContext tests the ability of the restore function to gracefully handle cases where errors // occur due to various reasons, such as context time-outs. The process should not panic in these situations. func TestExecuteRestoreWithTimedOutContext(t *testing.T) { // Set up local backup directory @@ -369,9 +370,13 @@ func TestExecuteRestoreWithTimedOutContext(t *testing.T) { // @executeRestoreFullBackup (https://github.com/vitessio/vitess/blob/main/go/vt/mysqlctl/builtinbackupengine.go#L806), // therefore, ExecuteRestore will still pass. Once fixed (https://github.com/vitessio/vitess/issues/12830) // the below asserts will start failing. - assert.NoError(t, err) + assert.Error(t, err) assert.True(t, ok) - assert.NotNil(t, bm) + assert.Nil(t, bm) + // error message can contain any combination of "context deadline exceeded" or "context canceled" + if !strings.Contains(err.Error(), "context canceled") && !strings.Contains(err.Error(), "context deadline exceeded") { + assert.Fail(t, "Test should failed with either `context canceled` or `context deadline exceeded`") + } } // needInnoDBRedoLogSubdir indicates whether we need to create a redo log subdirectory. diff --git a/go/vt/mysqlctl/mysqld.go b/go/vt/mysqlctl/mysqld.go index 19ab541a322..410619140ec 100644 --- a/go/vt/mysqlctl/mysqld.go +++ b/go/vt/mysqlctl/mysqld.go @@ -443,7 +443,7 @@ func (mysqld *Mysqld) startNoWait(ctx context.Context, cnf *Mycnf, mysqldArgs .. err := cmd.Wait() log.Infof("%v exit: %v", ts, err) - // The process exited. Trigger OnTerm callbacks, unless we were cancelled. + // The process exited. Trigger OnTerm callbacks, unless we were canceled. select { case <-cancel: default: diff --git a/go/vt/mysqlctl/query.go b/go/vt/mysqlctl/query.go index 348600e28ae..1063d1a20b7 100644 --- a/go/vt/mysqlctl/query.go +++ b/go/vt/mysqlctl/query.go @@ -134,7 +134,7 @@ func (mysqld *Mysqld) executeFetchContext(ctx context.Context, conn *dbconnpool. default: } - // The context expired or was cancelled. + // The context expired or was canceled. // Try to kill the connection to effectively cancel the ExecuteFetch(). connID := conn.ID() log.Infof("Mysqld.executeFetchContext(): killing connID %v due to timeout of query: %v", connID, query) @@ -147,7 +147,7 @@ func (mysqld *Mysqld) executeFetchContext(ctx context.Context, conn *dbconnpool. // Close the connection. Upon Recycle() it will be thrown out. conn.Close() // ExecuteFetch() may have succeeded before we tried to kill it. - // If ExecuteFetch() had returned because we cancelled it, + // If ExecuteFetch() had returned because we canceled it, // then executeErr would be an error like "MySQL has gone away". if executeErr == nil { return qr, executeErr