From cfcd5114c1ba235da48c587d538bd2488b2f9c75 Mon Sep 17 00:00:00 2001 From: Renan Rangel Date: Fri, 18 Oct 2024 10:36:44 -0700 Subject: [PATCH] PR feedback Signed-off-by: Renan Rangel --- go/ioutil/writer.go | 10 ++--- go/vt/mysqlctl/mysqlshellbackupengine.go | 5 ++- go/vt/mysqlctl/mysqlshellbackupengine_test.go | 38 ++++++------------- 3 files changed, 20 insertions(+), 33 deletions(-) diff --git a/go/ioutil/writer.go b/go/ioutil/writer.go index 6cfe38f7ea8..759ae1b7ab6 100644 --- a/go/ioutil/writer.go +++ b/go/ioutil/writer.go @@ -89,15 +89,15 @@ func (tw *meteredWriter) Write(p []byte) (int, error) { return tw.meter.measure(tw.Writer.Write, p) } -// MemoryBuffer implements io.WriteCloser using an in-memory buffer. -type MemoryBuffer struct { +// BytesBufferWriter implements io.WriteCloser using an in-memory buffer. +type BytesBufferWriter struct { *bytes.Buffer } -func (m MemoryBuffer) Close() error { +func (m BytesBufferWriter) Close() error { return nil } -func NewMemoryBuffer() MemoryBuffer { - return MemoryBuffer{bytes.NewBuffer(nil)} +func NewBytesBufferWriter() BytesBufferWriter { + return BytesBufferWriter{bytes.NewBuffer(nil)} } diff --git a/go/vt/mysqlctl/mysqlshellbackupengine.go b/go/vt/mysqlctl/mysqlshellbackupengine.go index 8c347a008f6..b7405ce7eaa 100644 --- a/go/vt/mysqlctl/mysqlshellbackupengine.go +++ b/go/vt/mysqlctl/mysqlshellbackupengine.go @@ -155,8 +155,9 @@ func (be *MySQLShellBackupEngine) ExecuteBackup(ctx context.Context, params Back lockAcquired := time.Now() // we will report how long we hold the lock for // we need to release the global read lock in case the backup fails to start and - // the lock wasn't released by releaseReadLock() yet - defer func() { _ = params.Mysqld.ReleaseGlobalReadLock(ctx) }() + // the lock wasn't released by releaseReadLock() yet. context might be expired, + // so we pass a new one. + defer func() { _ = params.Mysqld.ReleaseGlobalReadLock(context.Background()) }() posBeforeBackup, err := params.Mysqld.PrimaryPosition(ctx) if err != nil { diff --git a/go/vt/mysqlctl/mysqlshellbackupengine_test.go b/go/vt/mysqlctl/mysqlshellbackupengine_test.go index 6960f44ad94..67f27b5382e 100644 --- a/go/vt/mysqlctl/mysqlshellbackupengine_test.go +++ b/go/vt/mysqlctl/mysqlshellbackupengine_test.go @@ -22,7 +22,6 @@ import ( "fmt" "os" "path" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -335,6 +334,7 @@ func TestMySQLShellBackupEngine_ExecuteBackup_ReleaseLock(t *testing.T) { mysqlShellBackupBinaryName = originalBinary }() + logger := logutil.NewMemoryLogger() fakedb := fakesqldb.New(t) defer fakedb.Close() mysql := NewFakeMysqlDaemon(fakedb) @@ -343,6 +343,7 @@ func TestMySQLShellBackupEngine_ExecuteBackup_ReleaseLock(t *testing.T) { be := &MySQLShellBackupEngine{} params := BackupParams{ TabletAlias: "test", + Logger: logger, Mysqld: mysql, } bs := FakeBackupStorage{ @@ -350,9 +351,8 @@ func TestMySQLShellBackupEngine_ExecuteBackup_ReleaseLock(t *testing.T) { } t.Run("lock released if we see the mysqlsh lock being acquired", func(t *testing.T) { - logger := logutil.NewMemoryLogger() - params.Logger = logger - manifestBuffer := ioutil.NewMemoryBuffer() + logger.Clear() + manifestBuffer := ioutil.NewBytesBufferWriter() bs.StartBackupReturn.BackupHandle = &FakeBackupHandle{ Dir: t.TempDir(), AddFileReturn: FakeBackupHandleAddFileReturn{WriteCloser: manifestBuffer}, @@ -376,21 +376,14 @@ func TestMySQLShellBackupEngine_ExecuteBackup_ReleaseLock(t *testing.T) { require.Equal(t, mysqlShellBackupEngineName, manifest.BackupMethod) // did we notice the lock was release and did we release it ours as well? - errorLogged := false - for _, event := range logger.Events { - if strings.Contains(event.Value, "global read lock released after") { - errorLogged = true - } - } - - assert.True(t, errorLogged, "failed to release the global lock after mysqlsh") + require.Contains(t, logger.String(), "global read lock released after", + "failed to release the global lock after mysqlsh") }) t.Run("lock released if when we don't see mysqlsh released it", func(t *testing.T) { mysql.GlobalReadLock = false // clear lock status. - logger := logutil.NewMemoryLogger() - params.Logger = logger - manifestBuffer := ioutil.NewMemoryBuffer() + logger.Clear() + manifestBuffer := ioutil.NewBytesBufferWriter() bs.StartBackupReturn.BackupHandle = &FakeBackupHandle{ Dir: t.TempDir(), AddFileReturn: FakeBackupHandleAddFileReturn{WriteCloser: manifestBuffer}, @@ -409,21 +402,14 @@ func TestMySQLShellBackupEngine_ExecuteBackup_ReleaseLock(t *testing.T) { require.False(t, mysql.GlobalReadLock) // lock must be released. // make sure we are at least logging the lock wasn't able to be released earlier. - errorLogged := false - for _, event := range logger.Events { - if strings.Contains(event.Value, "could not release global lock earlier") { - errorLogged = true - } - } - - assert.True(t, errorLogged, "failed to log error message when unable to release lock during backup") + require.Contains(t, logger.String(), "could not release global lock earlier", + "failed to log error message when unable to release lock during backup") }) t.Run("lock released when backup fails", func(t *testing.T) { mysql.GlobalReadLock = false // clear lock status. - logger := logutil.NewMemoryLogger() - params.Logger = logger - manifestBuffer := ioutil.NewMemoryBuffer() + logger.Clear() + manifestBuffer := ioutil.NewBytesBufferWriter() bs.StartBackupReturn.BackupHandle = &FakeBackupHandle{ Dir: t.TempDir(), AddFileReturn: FakeBackupHandleAddFileReturn{WriteCloser: manifestBuffer},