Skip to content
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

[dbnode] Fix index flush recovery when previous flush attempts have failed #1574

Merged
Prev Previous commit
Next Next commit
Cache the result of the is valid checkpoint file
Rob Skillington committed Apr 19, 2019
commit 2a89613d476498d080144060bb65afd017db732f
42 changes: 34 additions & 8 deletions src/dbnode/persist/fs/files.go
Original file line number Diff line number Diff line change
@@ -73,14 +73,27 @@ var (

type fileOpener func(filePath string) (*os.File, error)

// LazyEvalBool is a boolean is lazily evaluated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean that is

type LazyEvalBool uint8

const (
// EvalNone indicated the boolean has not been evaluated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indicates

EvalNone LazyEvalBool = iota
// EvalTrue indicates the boolean has been evaluated to true.
EvalTrue
// EvalFalse indicates the boolean has been evaluated to false.
EvalFalse
)

// FileSetFile represents a set of FileSet files for a given block start
type FileSetFile struct {
ID FileSetFileIdentifier
AbsoluteFilepaths []string

CachedSnapshotTime time.Time
CachedSnapshotID uuid.UUID
filePathPrefix string
CachedSnapshotTime time.Time
CachedSnapshotID uuid.UUID
CachedHasCompleteCheckpointFile LazyEvalBool
filePathPrefix string
}

// SnapshotTimeAndID returns the snapshot time and id for the given FileSetFile.
@@ -89,9 +102,11 @@ func (f *FileSetFile) SnapshotTimeAndID() (time.Time, uuid.UUID, error) {
if f.IsZero() {
return time.Time{}, nil, errSnapshotTimeAndIDZero
}
if len(f.AbsoluteFilepaths) > 0 && !strings.Contains(f.AbsoluteFilepaths[0], snapshotDirName) {
if len(f.AbsoluteFilepaths) > 0 &&
!strings.Contains(f.AbsoluteFilepaths[0], snapshotDirName) {
return time.Time{}, nil, fmt.Errorf(
"tried to determine snapshot time and id of non-snapshot: %s", f.AbsoluteFilepaths[0])
"tried to determine snapshot time and id of non-snapshot: %s",
f.AbsoluteFilepaths[0])
}

if !f.CachedSnapshotTime.IsZero() || f.CachedSnapshotID != nil {
@@ -117,20 +132,31 @@ func (f FileSetFile) IsZero() bool {

// HasCompleteCheckpointFile returns a bool indicating whether the given set of
// fileset files has a checkpoint file.
func (f FileSetFile) HasCompleteCheckpointFile() bool {
func (f *FileSetFile) HasCompleteCheckpointFile() bool {
switch f.CachedHasCompleteCheckpointFile {
case EvalNone:
f.CachedHasCompleteCheckpointFile = f.evalHasCompleteCheckpointFile()
return f.HasCompleteCheckpointFile()
case EvalTrue:
return true
}
return false
}

func (f *FileSetFile) evalHasCompleteCheckpointFile() LazyEvalBool {
for _, fileName := range f.AbsoluteFilepaths {
if strings.Contains(fileName, checkpointFileSuffix) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably don't need this line anymore since CompleteCheckpointFileExists does this already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True but that will cause an "Invariant error" to be allocated which I'd like to avoid in the case that we're scanning a lot of files.

exists, err := CompleteCheckpointFileExists(fileName)
if err != nil {
continue
}
if exists {
return true
return EvalTrue
}
}
}

return false
return EvalFalse
}

// FileSetFilesSlice is a slice of FileSetFile
8 changes: 6 additions & 2 deletions src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go
Original file line number Diff line number Diff line change
@@ -47,8 +47,8 @@ import (
xsync "github.com/m3db/m3/src/x/sync"
xtime "github.com/m3db/m3/src/x/time"

"go.uber.org/zap"
"github.com/uber-go/tally"
"go.uber.org/zap"
)

var (
@@ -402,9 +402,13 @@ func (s *commitLogSource) mostRecentCompleteSnapshotByBlockShard(
// CachedSnapshotTime field so that we can rely upon it from here on out.
_, _, err := mostRecentSnapshotVolume.SnapshotTimeAndID()
if err != nil {
namespace := mostRecentSnapshot.ID.Namespace
if namespace == nil {
namespace = ident.StringID("<nil>")
}
s.log.
With(
zap.Stringer("namespace", mostRecentSnapshot.ID.Namespace),
zap.Stringer("namespace", namespace),
zap.Time("blockStart", mostRecentSnapshot.ID.BlockStart),
zap.Uint32("shard", mostRecentSnapshot.ID.Shard),
zap.Int("index", mostRecentSnapshot.ID.VolumeIndex),
Original file line number Diff line number Diff line change
@@ -324,8 +324,9 @@ func TestItMergesSnapshotsAndCommitLogs(t *testing.T) {
VolumeIndex: 0,
},
// Make sure path passes the "is snapshot" check in SnapshotTimeAndID method.
AbsoluteFilepaths: []string{"snapshots/checkpoint"},
CachedSnapshotTime: start.Add(time.Minute),
AbsoluteFilepaths: []string{"snapshots/checkpoint"},
CachedHasCompleteCheckpointFile: fs.EvalTrue,
CachedSnapshotTime: start.Add(time.Minute),
},
}, nil
}
25 changes: 15 additions & 10 deletions src/dbnode/storage/namespace_test.go
Original file line number Diff line number Diff line change
@@ -1268,8 +1268,9 @@ func TestNamespaceIsCapturedBySnapshot(t *testing.T) {
BlockStart: testTime.Truncate(blockSize),
},
// Must contain checkpoint file to be "valid".
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedSnapshotTime: testTime.Add(-1 * time.Second),
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedHasCompleteCheckpointFile: fs.EvalTrue,
CachedSnapshotTime: testTime.Add(-1 * time.Second),
},
},
},
@@ -1288,8 +1289,9 @@ func TestNamespaceIsCapturedBySnapshot(t *testing.T) {
BlockStart: blockStart,
},
// Must contain checkpoint file to be "valid".
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedSnapshotTime: testTime.Add(1 * time.Second),
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedHasCompleteCheckpointFile: fs.EvalTrue,
CachedSnapshotTime: testTime.Add(1 * time.Second),
},
},
},
@@ -1310,8 +1312,9 @@ func TestNamespaceIsCapturedBySnapshot(t *testing.T) {
BlockStart: blockStart,
},
// Must contain checkpoint file to be "valid".
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedSnapshotTime: testTime.Add(1 * time.Second),
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedHasCompleteCheckpointFile: fs.EvalTrue,
CachedSnapshotTime: testTime.Add(1 * time.Second),
},
},
},
@@ -1331,16 +1334,18 @@ func TestNamespaceIsCapturedBySnapshot(t *testing.T) {
BlockStart: blockStart,
},
// Must contain checkpoint file to be "valid".
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedSnapshotTime: testTime.Add(1 * time.Second),
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedHasCompleteCheckpointFile: fs.EvalTrue,
CachedSnapshotTime: testTime.Add(1 * time.Second),
},
fs.FileSetFile{
ID: fs.FileSetFileIdentifier{
BlockStart: blockStart.Add(blockSize),
},
// Must contain checkpoint file to be "valid".
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedSnapshotTime: testTime.Add(1 * time.Second),
AbsoluteFilepaths: []string{"snapshots-checkpoint"},
CachedHasCompleteCheckpointFile: fs.EvalTrue,
CachedSnapshotTime: testTime.Add(1 * time.Second),
},
},
},