Skip to content

Commit

Permalink
fixing bug vitessio#12830
Browse files Browse the repository at this point in the history
Signed-off-by: Rameez Sajwani <[email protected]>
  • Loading branch information
rsajwani committed Apr 6, 2023
1 parent 194968d commit f89442a
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 12 deletions.
8 changes: 4 additions & 4 deletions go/vt/mysqlctl/builtinbackupengine.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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")
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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:
Expand Down
15 changes: 10 additions & 5 deletions go/vt/mysqlctl/builtinbackupengine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"os"
"path"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion go/vt/mysqlctl/mysqld.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions go/vt/mysqlctl/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down

0 comments on commit f89442a

Please sign in to comment.