From 945ca7c4aa6073a2f385c523864d7c324c72fd73 Mon Sep 17 00:00:00 2001
From: Bo Du <bo@chronosphere.io>
Date: Thu, 23 Jul 2020 18:50:03 -0400
Subject: [PATCH] Cleanup coldflush manager and add a test.

---
 src/dbnode/storage/coldflush.go      |  8 ++-----
 src/dbnode/storage/coldflush_test.go | 34 ++++++++++++++++++++++++++++
 src/dbnode/storage/flush_test.go     |  3 ---
 src/dbnode/storage/mediator.go       |  2 +-
 4 files changed, 37 insertions(+), 10 deletions(-)
 create mode 100644 src/dbnode/storage/coldflush_test.go

diff --git a/src/dbnode/storage/coldflush.go b/src/dbnode/storage/coldflush.go
index 20eb022fe3..9d48b987e9 100644
--- a/src/dbnode/storage/coldflush.go
+++ b/src/dbnode/storage/coldflush.go
@@ -25,7 +25,6 @@ import (
 	"time"
 
 	"github.com/m3db/m3/src/dbnode/persist"
-	"github.com/m3db/m3/src/dbnode/persist/fs/commitlog"
 	xerrors "github.com/m3db/m3/src/x/errors"
 	"github.com/m3db/m3/src/x/instrument"
 
@@ -34,7 +33,6 @@ import (
 )
 
 type coldFlushManager struct {
-	databaseFlushManager
 	databaseCleanupManager
 	sync.RWMutex
 
@@ -52,16 +50,14 @@ type coldFlushManager struct {
 func newColdFlushManager(
 	database database,
 	pm persist.Manager,
-	commitLog commitlog.CommitLog,
 	opts Options,
 ) databaseColdFlushManager {
 	instrumentOpts := opts.InstrumentOptions()
 	scope := instrumentOpts.MetricsScope().SubScope("fs")
-	fm := newFlushManager(database, commitLog, scope)
-	cm := newCleanupManager(database, commitLog, scope)
+	// NB(bodu): cold flush cleanup doesn't require commit logs.
+	cm := newCleanupManager(database, nil, scope)
 
 	return &coldFlushManager{
-		databaseFlushManager:   fm,
 		databaseCleanupManager: cm,
 		log:                    instrumentOpts.Logger(),
 		database:               database,
diff --git a/src/dbnode/storage/coldflush_test.go b/src/dbnode/storage/coldflush_test.go
new file mode 100644
index 0000000000..bba6a2a83d
--- /dev/null
+++ b/src/dbnode/storage/coldflush_test.go
@@ -0,0 +1,34 @@
+package storage
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/m3db/m3/src/dbnode/persist"
+	"github.com/stretchr/testify/require"
+)
+
+func TestColdFlushManagerFlushDoneFlushError(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	var (
+		fakeErr            = errors.New("fake error while marking flush done")
+		mockPersistManager = persist.NewMockManager(ctrl)
+		mockFlushPersist   = persist.NewMockFlushPreparer(ctrl)
+	)
+
+	mockFlushPersist.EXPECT().DoneFlush().Return(fakeErr)
+	mockPersistManager.EXPECT().StartFlushPersist().Return(mockFlushPersist, nil)
+
+	testOpts := DefaultTestOptions().SetPersistManager(mockPersistManager)
+	db := newMockdatabase(ctrl)
+	db.EXPECT().Options().Return(testOpts).AnyTimes()
+	db.EXPECT().OwnedNamespaces().Return(nil, nil)
+
+	cfm := newColdFlushManager(db, mockPersistManager, testOpts).(*coldFlushManager)
+	cfm.pm = mockPersistManager
+
+	require.EqualError(t, fakeErr, cfm.coldFlush().Error())
+}
diff --git a/src/dbnode/storage/flush_test.go b/src/dbnode/storage/flush_test.go
index cf1704333c..1e271a7b3f 100644
--- a/src/dbnode/storage/flush_test.go
+++ b/src/dbnode/storage/flush_test.go
@@ -224,7 +224,6 @@ func TestFlushManagerNamespaceFlushTimesErr(t *testing.T) {
 	ns.EXPECT().Options().Return(nsOpts).AnyTimes()
 	ns.EXPECT().ID().Return(defaultTestNs1ID).AnyTimes()
 	ns.EXPECT().NeedsFlush(gomock.Any(), gomock.Any()).Return(false, fakeErr).AnyTimes()
-	ns.EXPECT().ColdFlush(gomock.Any()).Return(nil).AnyTimes()
 	ns.EXPECT().Snapshot(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 	db.EXPECT().OwnedNamespaces().Return([]databaseNamespace{ns}, nil)
 
@@ -322,7 +321,6 @@ func TestFlushManagerSkipNamespaceIndexingDisabled(t *testing.T) {
 	ns.EXPECT().ID().Return(defaultTestNs1ID).AnyTimes()
 	ns.EXPECT().NeedsFlush(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes()
 	ns.EXPECT().WarmFlush(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
-	ns.EXPECT().ColdFlush(gomock.Any()).Return(nil).AnyTimes()
 	ns.EXPECT().Snapshot(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 
 	var (
@@ -366,7 +364,6 @@ func TestFlushManagerNamespaceIndexingEnabled(t *testing.T) {
 	ns.EXPECT().ID().Return(defaultTestNs1ID).AnyTimes()
 	ns.EXPECT().NeedsFlush(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes()
 	ns.EXPECT().WarmFlush(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
-	ns.EXPECT().ColdFlush(gomock.Any()).Return(nil).AnyTimes()
 	ns.EXPECT().Snapshot(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 	ns.EXPECT().FlushIndex(gomock.Any()).Return(nil)
 
diff --git a/src/dbnode/storage/mediator.go b/src/dbnode/storage/mediator.go
index 14dc654590..527ba5897d 100644
--- a/src/dbnode/storage/mediator.go
+++ b/src/dbnode/storage/mediator.go
@@ -119,7 +119,7 @@ func newMediator(database database, commitlog commitlog.CommitLog, opts Options)
 	if err != nil {
 		return nil, err
 	}
-	cfm := newColdFlushManager(database, pm, commitlog, opts)
+	cfm := newColdFlushManager(database, pm, opts)
 	d.databaseColdFlushManager = cfm
 
 	d.databaseRepairer = newNoopDatabaseRepairer()