From 4bf09d5eedc8ef22b1cffe18c7b770c0532f0a44 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 11 Mar 2024 09:45:30 -0700 Subject: [PATCH] manifest: improve VersionEdit tests In this commit we add a `ParseVersionEditDebug` which can create a `VersionEdit` from a `DebugString` output (similar to `ParseFileMetadataDebug` and `ParseVersionDebug`). We improve `TestVersionEditApply` to use the `DebugString` formats instead of a one-off format. --- internal/manifest/testdata/version_edit_apply | 275 +++++++++--------- internal/manifest/testutils.go | 19 ++ internal/manifest/version.go | 11 +- internal/manifest/version_edit.go | 50 ++++ internal/manifest/version_edit_test.go | 178 ++++++------ 5 files changed, 296 insertions(+), 237 deletions(-) diff --git a/internal/manifest/testdata/version_edit_apply b/internal/manifest/testdata/version_edit_apply index c8c1be43e9..55632c1a0d 100644 --- a/internal/manifest/testdata/version_edit_apply +++ b/internal/manifest/testdata/version_edit_apply @@ -1,180 +1,171 @@ -apply - L0 - 1:[a#1,SET-b#2,SET] - 2:[c#3,SET-d#4,SET] -edit - delete - L0 - 1 - add - L2 - 1:[a#1,SET-b#2,SET] - 4:[c#3,SET-d#4,SET] +define v1 +L0: + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] + 000001:[a#1,SET-b#2,SET] seqnums:[1-2] ---- L0.0: - 000002:[c#3,SET-d#4,SET] + 000001:[a#1,SET-b#2,SET] seqnums:[1-2] points:[a#1,SET-b#2,SET] + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] + +# Empty edit. +apply v1 +---- +L0.0: + 000001:[a#1,SET-b#2,SET] seqnums:[1-2] points:[a#1,SET-b#2,SET] + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] + +apply v1 + deleted: L0 000001 + added: L2 000001:[a#1,SET-b#2,SET] seqnums:[1-2] + added: L2 000004:[c#3,SET-d#4,SET] seqnums:[3-4] +---- +L0.0: + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] L2: - 000001:[a#1,SET-b#2,SET] - 000004:[c#3,SET-d#4,SET] + 000001:[a#1,SET-b#2,SET] seqnums:[1-2] points:[a#1,SET-b#2,SET] + 000004:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] -apply - L0 - 1:[a#1,SET-b#2,SET] - 2:[c#3,SET-d#4,SET] -edit - delete - L1 - 1 +apply v1 + deleted: L1 000001 ---- pebble: internal error: No current or added files but have deleted files: 1 -apply - L0 - 1:[a#1,SET-c#2,SET] - 2:[c#3,SET-d#4,SET] -edit - delete - L0 - 1 - add - L2 - 1:[a#1,SET-c#2,SET] - 4:[b#3,SET-d#4,SET] ----- -pebble: internal error: L2 files 000001 and 000004 have overlapping ranges: [a#1,SET-c#2,SET] vs [b#3,SET-d#4,SET] - -apply - L0 - 1:[a#1,SET-c#2,SET] - 2:[c#3,SET-d#4,SET] -edit - add - L0 - 4:[b#3,SET-d#5,SET] +apply v1 + deleted: L0 000001 + added: L2 000001:[a#1,SET-b#2,SET] seqnums:[1-2] + added: L2 000004:[b#3,SET-d#4,SET] seqnums:[3-4] +---- +pebble: internal error: L2 files 000001 and 000004 have overlapping ranges: [a#1,SET-b#2,SET] vs [b#3,SET-d#4,SET] + +define v2 +L0: + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] + 000001:[a#1,SET-c#2,SET] seqnums:[1-2] +---- +L0.1: + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] +L0.0: + 000001:[a#1,SET-c#2,SET] seqnums:[1-2] points:[a#1,SET-c#2,SET] + +apply v2 + added: L0 000004:[b#3,SET-d#5,SET] seqnums:[3-5] ---- L0.2: - 000004:[b#3,SET-d#5,SET] + 000004:[b#3,SET-d#5,SET] seqnums:[3-5] points:[b#3,SET-d#5,SET] L0.1: - 000002:[c#3,SET-d#4,SET] + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] L0.0: - 000001:[a#1,SET-c#2,SET] + 000001:[a#1,SET-c#2,SET] seqnums:[1-2] points:[a#1,SET-c#2,SET] -apply - L0 - 1:[a#1,SET-c#2,SET] - 2:[c#3,SET-d#4,SET] -edit - add - L0 - 4:[b#0,SET-d#0,SET] +apply v2 + added: L0 000004:[b#0,SET-d#0,SET] seqnums:[0-0] ---- L0.2: - 000002:[c#3,SET-d#4,SET] + 000002:[c#3,SET-d#4,SET] seqnums:[3-4] points:[c#3,SET-d#4,SET] L0.1: - 000001:[a#1,SET-c#2,SET] + 000001:[a#1,SET-c#2,SET] seqnums:[1-2] points:[a#1,SET-c#2,SET] L0.0: - 000004:[b#0,SET-d#0,SET] + 000004:[b#0,SET-d#0,SET] seqnums:[0-0] points:[b#0,SET-d#0,SET] + +define empty +---- -apply -edit - add - L0 - 1:[a#1,SET-c#2,SET] - 4:[b#3,SET-d#5,SET] +apply empty + added: L0 000001:[a#1,SET-c#2,SET] seqnums:[1-2] + added: L0 000004:[b#3,SET-d#5,SET] seqnums:[3-5] ---- L0.1: - 000004:[b#3,SET-d#5,SET] + 000004:[b#3,SET-d#5,SET] seqnums:[3-5] points:[b#3,SET-d#5,SET] L0.0: - 000001:[a#1,SET-c#2,SET] + 000001:[a#1,SET-c#2,SET] seqnums:[1-2] points:[a#1,SET-c#2,SET] + +apply v2 + added: L0 000001:[a#1,SET-c#2,SET] seqnums:[1-2] +---- +pebble: files 000001 and 000001 collided on sort keys -apply - L0 - 1:[a#1,SET-c#2,SET] +apply empty + added: L0 000001:[a#1,SET-c#2,SET] seqnums:[1-2] ---- L0.0: - 000001:[a#1,SET-c#2,SET] - -apply - L2 - 3:[b#1,SET-c#2,SET] - 4:[d#3,SET-f#4,SET] - 5:[h#3,SET-h#2,SET] - 2:[n#5,SET-q#3,SET] - 1:[r#2,SET-t#1,SET] -edit - delete - L2 - 4 - 1 - add - L2 - 6:[a#10,SET-a#7,SET] - 7:[e#1,SET-g#2,SET] - 10:[j#3,SET-m#2,SET] + 000001:[a#1,SET-c#2,SET] seqnums:[1-2] points:[a#1,SET-c#2,SET] + +apply empty + added: L2 000010:[j#3,SET-m#2,SET] + added: L2 000006:[a#10,SET-a#7,SET] ---- L2: - 000006:[a#10,SET-a#7,SET] + 000006:[a#10,SET-a#7,SET] seqnums:[0-0] points:[a#10,SET-a#7,SET] + 000010:[j#3,SET-m#2,SET] seqnums:[0-0] points:[j#3,SET-m#2,SET] + +define v3 +L2: 000003:[b#1,SET-c#2,SET] - 000007:[e#1,SET-g#2,SET] + 000004:[d#3,SET-f#4,SET] 000005:[h#3,SET-h#2,SET] - 000010:[j#3,SET-m#2,SET] 000002:[n#5,SET-q#3,SET] + 000001:[r#2,SET-t#1,SET] +---- +L2: + 000003:[b#1,SET-c#2,SET] seqnums:[0-0] points:[b#1,SET-c#2,SET] + 000004:[d#3,SET-f#4,SET] seqnums:[0-0] points:[d#3,SET-f#4,SET] + 000005:[h#3,SET-h#2,SET] seqnums:[0-0] points:[h#3,SET-h#2,SET] + 000002:[n#5,SET-q#3,SET] seqnums:[0-0] points:[n#5,SET-q#3,SET] + 000001:[r#2,SET-t#1,SET] seqnums:[0-0] points:[r#2,SET-t#1,SET] -apply -edit - add - L2 - 10:[j#3,SET-m#2,SET] - 6:[a#10,SET-a#7,SET] +apply v3 + deleted: L2 000004 + deleted: L2 000001 + added: L2 000006:[a#10,SET-a#7,SET] + added: L2 000007:[e#1,SET-g#2,SET] + added: L2 000010:[j#3,SET-m#2,SET] ---- L2: - 000006:[a#10,SET-a#7,SET] - 000010:[j#3,SET-m#2,SET] - -apply - L0 - 1:[a#1,SET-b#2,SET] - L1 - 2:[c#3,SET-d#2,SET] -edit - delete - L0 - 1 - L1 - 2 + 000006:[a#10,SET-a#7,SET] seqnums:[0-0] points:[a#10,SET-a#7,SET] + 000003:[b#1,SET-c#2,SET] seqnums:[0-0] points:[b#1,SET-c#2,SET] + 000007:[e#1,SET-g#2,SET] seqnums:[0-0] points:[e#1,SET-g#2,SET] + 000005:[h#3,SET-h#2,SET] seqnums:[0-0] points:[h#3,SET-h#2,SET] + 000010:[j#3,SET-m#2,SET] seqnums:[0-0] points:[j#3,SET-m#2,SET] + 000002:[n#5,SET-q#3,SET] seqnums:[0-0] points:[n#5,SET-q#3,SET] + +define v4 +L0: + 000001:[a#1,SET-b#2,SET] +L1: + 000002:[c#3,SET-d#2,SET] +---- +L0.0: + 000001:[a#1,SET-b#2,SET] seqnums:[0-0] points:[a#1,SET-b#2,SET] +L1: + 000002:[c#3,SET-d#2,SET] seqnums:[0-0] points:[c#3,SET-d#2,SET] + +apply v4 + deleted: L0 000001 + deleted: L1 000002 ---- # Deletion of a non-existent table results in an error. +apply v4 + deleted: L0 000004 +---- +error during Accumulate: pebble: file deleted L0.000004 before it was inserted + +define v5 +L0: + 000001:[a#1,SET-b#2,SET] +---- +L0.0: + 000001:[a#1,SET-b#2,SET] seqnums:[0-0] points:[a#1,SET-b#2,SET] -apply - L0 - 1:[a#1,SET-b#2,SET] -edit - delete - L0 - 2 ----- -pebble: file deleted L0.000002 before it was inserted - -apply - L0 - 1:[a#1,SET-b#2,SET] -edit - delete - L0 - 1 - add - L2 - 1:[a#1,SET-b#2,SET] - 4:[c#3,SET-d#4,SET] - 5:[s#3,SET-z#4,SET] -edit - delete - L2 - 1 - L2 - 4 +apply v5 + deleted: L0 1 + added: L2 000001:[a#1,SET-b#2,SET] + added: L2 000004:[c#3,SET-d#4,SET] + added: L2 000005:[s#3,SET-z#4,SET] +new version edit + deleted: L2 000001 + deleted: L2 000004 ---- L2: - 000005:[s#3,SET-z#4,SET] + 000005:[s#3,SET-z#4,SET] seqnums:[0-0] points:[s#3,SET-z#4,SET] diff --git a/internal/manifest/testutils.go b/internal/manifest/testutils.go index 1e6b60e2dc..7f543b9918 100644 --- a/internal/manifest/testutils.go +++ b/internal/manifest/testutils.go @@ -78,6 +78,13 @@ func (p *debugParser) Next() string { return res } +// Remaining returns all the remaining tokens, separated by spaces. +func (p *debugParser) Remaining() string { + res := strings.Join(p.tokens, " ") + p.tokens = nil + return res +} + // Expect consumes the next tokens, verifying that they exactly match the // arguments. func (p *debugParser) Expect(tokens ...string) { @@ -142,3 +149,15 @@ func (p *debugParser) Errf(format string, args ...any) { msg := fmt.Sprintf(format, args...) panic(errors.Errorf("error parsing %q at token %q: %s", p.original, p.lastToken, msg)) } + +// maybeRecover can be used in a defer to convert panics into errors. +func maybeRecover() error { + if r := recover(); r != nil { + err, ok := r.(error) + if !ok { + err = errors.Errorf("%v", r) + } + return err + } + return nil +} diff --git a/internal/manifest/version.go b/internal/manifest/version.go index 553dc17d26..eb037af8fd 100644 --- a/internal/manifest/version.go +++ b/internal/manifest/version.go @@ -814,13 +814,7 @@ func (m *FileMetadata) DebugString(format base.FormatKey, verbose bool) string { // representation. func ParseFileMetadataDebug(s string) (_ *FileMetadata, err error) { defer func() { - if r := recover(); r != nil { - var ok bool - err, ok = r.(error) - if !ok { - err = errors.Errorf("%v", r) - } - } + err = errors.CombineErrors(err, maybeRecover()) }() // Input format: @@ -1256,6 +1250,9 @@ func ParseVersionDebug(comparer *base.Comparer, flushSplitBytes int64, s string) var files [NumLevels][]*FileMetadata level := -1 for _, l := range strings.Split(s, "\n") { + if l == "" { + continue + } p := makeDebugParser(l) if l, ok := p.TryLevel(); ok { level = l diff --git a/internal/manifest/version_edit.go b/internal/manifest/version_edit.go index d32f5266da..2f9410df87 100644 --- a/internal/manifest/version_edit.go +++ b/internal/manifest/version_edit.go @@ -12,6 +12,7 @@ import ( "fmt" "io" "slices" + "strings" "time" "github.com/cockroachdb/errors" @@ -546,6 +547,55 @@ func (v *VersionEdit) String() string { return v.string(false /* verbose */, base.DefaultFormatter) } +// ParseVersionEditDebug parses a VersionEdit from its DebugString +// implementation. +// +// It doesn't recognize all fields; this implementation can be filled in as +// needed. +func ParseVersionEditDebug(s string) (_ *VersionEdit, err error) { + defer func() { + err = errors.CombineErrors(err, maybeRecover()) + }() + + var ve VersionEdit + for _, l := range strings.Split(s, "\n") { + l = strings.TrimSpace(l) + if l == "" { + continue + } + p := makeDebugParser(l) + field := p.Next() + p.Expect(":") + switch field { + case "added": + level := p.Level() + meta, err := ParseFileMetadataDebug(p.Remaining()) + if err != nil { + return nil, err + } + ve.NewFiles = append(ve.NewFiles, NewFileEntry{ + Level: level, + Meta: meta, + }) + + case "deleted": + level := p.Level() + num := p.FileNum() + if ve.DeletedFiles == nil { + ve.DeletedFiles = make(map[DeletedFileEntry]*FileMetadata) + } + ve.DeletedFiles[DeletedFileEntry{ + Level: level, + FileNum: num, + }] = nil + + default: + return nil, errors.Errorf("field %q not implemented", field) + } + } + return &ve, nil +} + // Encode encodes an edit to the specified writer. func (v *VersionEdit) Encode(w io.Writer) error { e := versionEditEncoder{new(bytes.Buffer)} diff --git a/internal/manifest/version_edit_test.go b/internal/manifest/version_edit_test.go index 8c1e0adbe2..294be1078e 100644 --- a/internal/manifest/version_edit_test.go +++ b/internal/manifest/version_edit_test.go @@ -10,7 +10,7 @@ import ( "io" "os" "reflect" - "strconv" + "slices" "strings" "testing" @@ -427,112 +427,114 @@ func TestVersionEditEncodeLastSeqNum(t *testing.T) { } func TestVersionEditApply(t *testing.T) { - parseMeta := func(s string) (*FileMetadata, error) { - m, err := ParseFileMetadataDebug(s) - if err != nil { - return nil, err - } - m.SmallestSeqNum = m.Smallest.SeqNum() - m.LargestSeqNum = m.Largest.SeqNum() - if m.SmallestSeqNum > m.LargestSeqNum { - m.SmallestSeqNum, m.LargestSeqNum = m.LargestSeqNum, m.SmallestSeqNum - } - m.InitPhysicalBacking() - return m, nil - } + const flushSplitBytes = 10 * 1024 * 1024 + const readCompactionRate = 32000 - // TODO(bananabrick): Improve the parsing logic in this test. + versions := make(map[string]*Version) datadriven.RunTest(t, "testdata/version_edit_apply", func(t *testing.T, d *datadriven.TestData) string { switch d.Cmd { - case "apply": - // TODO(sumeer): move this Version parsing code to utils, to - // avoid repeating it, and make it the inverse of - // Version.DebugString(). - var v *Version - var veList []*VersionEdit - isVersion := true - isDelete := true - var level int - var err error - versionFiles := map[base.FileNum]*FileMetadata{} - for _, data := range strings.Split(d.Input, "\n") { - data = strings.TrimSpace(data) - switch data { - case "edit": - isVersion = false - veList = append(veList, &VersionEdit{}) - case "delete": - isVersion = false - isDelete = true - case "add": - isVersion = false - isDelete = false - case "L0", "L1", "L2", "L3", "L4", "L5", "L6": - level, err = strconv.Atoi(data[1:]) - if err != nil { - return err.Error() - } - default: - var ve *VersionEdit - if len(veList) > 0 { - ve = veList[len(veList)-1] - } - if isVersion || !isDelete { - meta, err := parseMeta(data) - if err != nil { - return err.Error() - } - if isVersion { - if v == nil { - v = &Version{cmp: base.DefaultComparer} - for l := 0; l < NumLevels; l++ { - v.Levels[l] = makeLevelMetadata(base.DefaultComparer.Compare, l, nil /* files */) - } - } - versionFiles[meta.FileNum] = meta - v.Levels[level].insert(meta) - meta.LatestRef() - } else { - ve.NewFiles = - append(ve.NewFiles, NewFileEntry{Level: level, Meta: meta}) - } - } else { - fileNum, err := strconv.Atoi(data) - if err != nil { - return err.Error() - } - dfe := DeletedFileEntry{Level: level, FileNum: base.FileNum(fileNum)} - if ve.DeletedFiles == nil { - ve.DeletedFiles = make(map[DeletedFileEntry]*FileMetadata) - } - ve.DeletedFiles[dfe] = versionFiles[dfe.FileNum] - } - } + case "define": + // Define a version. + name := d.CmdArgs[0].String() + v, err := ParseVersionDebug(base.DefaultComparer, flushSplitBytes, d.Input) + if err != nil { + d.Fatalf(t, "%v", err) } + versions[name] = v + return v.DebugString() - const flushSplitBytes = 10 * 1024 * 1024 - if v != nil { - if err := v.InitL0Sublevels(flushSplitBytes); err != nil { - return err.Error() - } + case "apply": + // Apply an edit to the given version. + name := d.CmdArgs[0].String() + v := versions[name] + if v == nil { + d.Fatalf(t, "unknown version %q", name) + return "" } bve := BulkVersionEdit{} bve.AddedByFileNum = make(map[base.FileNum]*FileMetadata) - for _, ve := range veList { + for _, l := range v.Levels { + it := l.Iter() + for f := it.First(); f != nil; f = it.Next() { + bve.AddedByFileNum[f.FileNum] = f + } + } + + lines := strings.Split(d.Input, "\n") + for len(lines) > 0 { + // We allow multiple version edits with a special separator. + var linesForOneVE []string + if nextSplit := slices.Index(lines, "new version edit"); nextSplit != -1 { + linesForOneVE = lines[:nextSplit] + lines = lines[nextSplit+1:] + } else { + linesForOneVE = lines + lines = nil + } + ve, err := ParseVersionEditDebug(strings.Join(linesForOneVE, "\n")) + if err != nil { + d.Fatalf(t, "%v", err) + } if err := bve.Accumulate(ve); err != nil { - return err.Error() + return fmt.Sprintf("error during Accumulate: %v", err) } } - newv, err := bve.Apply(v, base.DefaultComparer, flushSplitBytes, 32000) + + newv, err := bve.Apply(v, base.DefaultComparer, flushSplitBytes, readCompactionRate) if err != nil { return err.Error() } - return newv.String() + + // Reinitialize the L0 sublevels in the original version; otherwise we + // will get "AddL0Files called twice on the same receiver" panics. + if err := v.InitL0Sublevels(flushSplitBytes); err != nil { + panic(err) + } + return newv.DebugString() default: return fmt.Sprintf("unknown command: %s", d.Cmd) } }) } + +func TestParseVersionEditDebugRoundTrip(t *testing.T) { + testCases := []struct { + input string + output string + }{ + { + input: ` added: L1 000001:[a#0,SET-z#0,DEL] seqnums:[0-0] points:[a#0,SET-z#0,DEL]`, + }, + { + input: `added: L0 1:[a#0,SET-z#0,DEL]`, + output: ` added: L0 000001:[a#0,SET-z#0,DEL] seqnums:[0-0] points:[a#0,SET-z#0,DEL]`, + }, + { + input: ` deleted: L1 000001`, + }, + { + input: strings.Join([]string{ + ` deleted: L1 000002`, + ` deleted: L3 000003`, + ` added: L1 000001:[a#0,SET-z#0,DEL] seqnums:[0-0] points:[a#0,SET-z#0,DEL]`, + ` added: L2 000002:[a#0,SET-z#0,DEL] seqnums:[0-0] points:[a#0,SET-z#0,DEL]`, + }, "\n"), + }, + } + for _, tc := range testCases { + t.Run("", func(t *testing.T) { + ve, err := ParseVersionEditDebug(tc.input) + require.NoError(t, err) + got := ve.DebugString(base.DefaultFormatter) + got = strings.TrimSuffix(got, "\n") + want := tc.input + if tc.output != "" { + want = tc.output + } + require.Equal(t, want, got) + }) + } +}