db,valsep: move valsep tests to appropriate pkg

Author: xinhaoz

blob_rewrite_test.go

@@ -67,7 +67,7 @@ func TestBlobRewrite(t *testing.T) {
 		tw = sstable.NewRawWriter(w, sstable.WriterOptions{
 			TableFormat: sstable.TableFormatMax,
 		})
-		tw = &loggingRawWriter{w: &buf, RawWriter: tw}
+		tw = &sstable.LoggingRawWriter{LogWriter: &buf, RawWriter: tw}
 	}
 
 	datadriven.RunTest(t, "testdata/blob_rewrite",

compaction.go

@@ -235,7 +235,7 @@ type tableCompaction struct {
 	// resulting version has been installed (if successful), but the compaction
 	// goroutine is still cleaning up (eg, deleting obsolete files).
 	versionEditApplied bool
-	// getValueSeparation constructs a compact.ValueSeparation for use in a
+	// getValueSeparation constructs a valsep.ValueSeparation for use in a
 	// compaction. It implements heuristics around choosing whether a compaction
 	// should:
 	//

compaction_value_separation.go

@@ -24,7 +24,7 @@ var neverSeparateValues getValueSeparation = func(JobID, *tableCompaction, Value
 }
 
 // determineCompactionValueSeparation determines whether a compaction should
-// separate values into blob files. It returns a compact.ValueSeparation
+// separate values into blob files. It returns a valsep.ValueSeparation
 // implementation that should be used for the compaction.
 //
 // It assumes that the compaction will write tables at d.TableFormat() or above.

data_test.go

@@ -905,11 +905,12 @@ func runDBDefineCmd(td *datadriven.TestData, opts *Options) (*DB, error) {
 	}
 	d.mu.versions.dynamicBaseLevel = false
 
-	// We use a custom compact.ValueSeparation implementation that wraps
-	// preserveBlobReferences. It parses values containing human-readable blob
-	// references (as understood by blobtest.Values) and preserves the
-	// references.  It also accumulates all the handles so that after writing
-	// all the tables, we can construct all the referenced blob files and add
+	// We use a custom valsep.ValueSeparation implementation that wraps
+	// valsep.ValueSeparator in preserveAllHotBlobReferences mode. It
+	// parses values containing human-readable blob references (as
+	// understood by blobtest.Values) and preserves the references. It
+	// also accumulates all the handles so that after writing all the
+	// tables, we can construct all the referenced blob files and add
 	// them to the final version edit.
 	valueSeparator := &defineDBValueSeparator{
 		metas: make(map[base.BlobFileID]*manifest.PhysicalBlobFile),
@@ -1922,3 +1923,89 @@ func streamFilterBetweenGrep(start, end string) stream.Filter {
 		return nil
 	})
 }
+
+// defineDBValueSeparator is a valsep.ValueSeparation implementation used by
+// datadriven tests when defining a database state. It is a wrapper around
+// preserveBlobReferences that also parses string representations of blob
+// references from values.
+type defineDBValueSeparator struct {
+	bv    blobtest.Values
+	metas map[base.BlobFileID]*manifest.PhysicalBlobFile
+	pbr   valsep.ValueSeparation
+	kv    base.InternalKV
+}
+
+// Assert that *defineDBValueSeparator implements the valsep.ValueSeparation interface.
+var _ valsep.ValueSeparation = (*defineDBValueSeparator)(nil)
+
+// SetNextOutputConfig implements the valsep.ValueSeparation interface.
+func (vs *defineDBValueSeparator) SetNextOutputConfig(config valsep.ValueSeparationOutputConfig) {}
+
+// Kind implements the ValueSeparation interface.
+func (vs *defineDBValueSeparator) Kind() sstable.ValueSeparationKind {
+	return vs.pbr.Kind()
+}
+
+// MinimumSize implements the ValueSeparation interface.
+func (vs *defineDBValueSeparator) MinimumSize() int { return vs.pbr.MinimumSize() }
+
+// EstimatedFileSize returns an estimate of the disk space consumed by the current
+// blob file if it were closed now.
+func (vs *defineDBValueSeparator) EstimatedFileSize() uint64 {
+	return vs.pbr.EstimatedFileSize()
+}
+
+// EstimatedReferenceSize returns an estimate of the disk space consumed by the
+// current output sstable's blob references so far.
+func (vs *defineDBValueSeparator) EstimatedReferenceSize() uint64 {
+	return vs.pbr.EstimatedReferenceSize()
+}
+
+// Add adds the provided key-value pair to the sstable, possibly separating the
+// value into a blob file.
+func (vs *defineDBValueSeparator) Add(
+	tw sstable.RawWriter, kv *base.InternalKV, forceObsolete bool, _ bool,
+) error {
+	// In datadriven tests, all defined values are in-place initially. See
+	// runDBDefineCmdReuseFS.
+	v := kv.V.InPlaceValue()
+	// If the value doesn't begin with "blob", don't separate it.
+	if !bytes.HasPrefix(v, []byte("blob")) {
+		return tw.Add(kv.K, v, forceObsolete)
+	}
+
+	// This looks like a blob reference. Parse it.
+	iv, err := vs.bv.ParseInternalValue(string(v))
+	if err != nil {
+		return err
+	}
+	lv := iv.LazyValue()
+	// If we haven't seen this blob file before, fabricate a metadata for it.
+	fileID := lv.Fetcher.BlobFileID
+	meta, ok := vs.metas[fileID]
+	if !ok {
+		meta = &manifest.PhysicalBlobFile{
+			FileNum:      base.DiskFileNum(fileID),
+			CreationTime: uint64(time.Now().Unix()),
+		}
+		vs.metas[fileID] = meta
+	}
+	meta.Size += uint64(lv.Fetcher.Attribute.ValueLen)
+	meta.ValueSize += uint64(lv.Fetcher.Attribute.ValueLen)
+
+	// Return a KV that uses the original key but our constructed blob reference.
+	vs.kv.K = kv.K
+	vs.kv.V = iv
+	return vs.pbr.Add(tw, &vs.kv, forceObsolete, false /* isLikelyMVCCGarbage */)
+}
+
+// FinishOutput implements valsep.ValueSeparation.
+func (d *defineDBValueSeparator) FinishOutput() (valsep.ValueSeparationMetadata, error) {
+	m, err := d.pbr.FinishOutput()
+	if err != nil {
+		return valsep.ValueSeparationMetadata{}, err
+	}
+	// TODO(jackson): Support setting a specific depth from the datadriven test.
+	m.BlobReferenceDepth = manifest.BlobReferenceDepth(len(m.BlobReferences))
+	return m, nil
+}

sstable/writer.go

@@ -6,6 +6,8 @@ package sstable
 
 import (
 	"context"
+	"fmt"
+	"io"
 
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble/internal/base"
@@ -470,3 +472,23 @@ func (m *WriterMetadata) updateSeqNum(seqNum base.SeqNum) {
 		m.LargestSeqNum = seqNum
 	}
 }
+
+// LoggingRawWriter wraps a sstable.RawWriter and logs calls to Add and
+// AddWithBlobHandle to provide observability into the separation of values into
+// blob files. This is intended for testing.
+type LoggingRawWriter struct {
+	LogWriter io.Writer
+	RawWriter
+}
+
+func (w *LoggingRawWriter) Add(key base.InternalKey, value []byte, forceObsolete bool) error {
+	fmt.Fprintf(w.LogWriter, "RawWriter.Add(%q, %q, %t)\n", key, value, forceObsolete)
+	return w.RawWriter.Add(key, value, forceObsolete)
+}
+
+func (w *LoggingRawWriter) AddWithBlobHandle(
+	key base.InternalKey, h blob.InlineHandle, attr base.ShortAttribute, forceObsolete bool,
+) error {
+	fmt.Fprintf(w.LogWriter, "RawWriter.AddWithBlobHandle(%q, %q, %x, %t)\n", key, h, attr, forceObsolete)
+	return w.RawWriter.AddWithBlobHandle(key, h, attr, forceObsolete)
+}

testdata/value_separation_policy renamed to valsep/testdata/value_separation_policy

@@ -2,16 +2,14 @@
 // of this source code is governed by a BSD-style license that can be found in
 // the LICENSE file.
 
-package pebble
+package valsep
 
 import (
 	"bytes"
 	"context"
 	"fmt"
-	"io"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/cockroachdb/crlib/crstrings"
 	"github.com/cockroachdb/datadriven"
@@ -24,15 +22,14 @@ import (
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
 	"github.com/cockroachdb/pebble/sstable"
 	"github.com/cockroachdb/pebble/sstable/blob"
-	"github.com/cockroachdb/pebble/valsep"
 	"github.com/cockroachdb/pebble/vfs"
 	"github.com/stretchr/testify/require"
 )
 
 func TestValueSeparationPolicy(t *testing.T) {
 	var (
 		bv  blobtest.Values
-		vs  valsep.ValueSeparation
+		vs  ValueSeparation
 		tw  sstable.RawWriter
 		fn  base.DiskFileNum
 		buf bytes.Buffer
@@ -62,7 +59,7 @@ func TestValueSeparationPolicy(t *testing.T) {
 		tw = sstable.NewRawWriter(w, sstable.WriterOptions{
 			TableFormat: sstable.TableFormatPebblev6,
 		})
-		tw = &loggingRawWriter{w: &buf, RawWriter: tw}
+		tw = &sstable.LoggingRawWriter{LogWriter: &buf, RawWriter: tw}
 	}
 
 	datadriven.RunTest(t, "testdata/value_separation_policy",
@@ -76,7 +73,7 @@ func TestValueSeparationPolicy(t *testing.T) {
 				bv = blobtest.Values{}
 				switch x := d.CmdArgs[0].String(); x {
 				case "never-separate-values":
-					vs = valsep.NeverSeparateValues{}
+					vs = NeverSeparateValues{}
 				case "preserve-blob-references":
 					lines := crstrings.Lines(d.Input)
 					inputBlobPhysicalFiles := make(map[base.BlobFileID]*manifest.PhysicalBlobFile, len(lines))
@@ -86,7 +83,7 @@ func TestValueSeparationPolicy(t *testing.T) {
 						fn = max(fn, bfm.Physical.FileNum)
 						inputBlobPhysicalFiles[bfm.FileID] = bfm.Physical
 					}
-					pbr := valsep.NewPreserveAllHotBlobReferences(
+					pbr := NewPreserveAllHotBlobReferences(
 						inputBlobPhysicalFiles, /* blob file set */
 						manifest.BlobReferenceDepth(0),
 						sstable.ValueSeparationDefault,
@@ -105,15 +102,15 @@ func TestValueSeparationPolicy(t *testing.T) {
 							t.Fatalf("unknown short attribute extractor: %s", arg.String())
 						}
 					}
-					newSep := valsep.NewWriteNewBlobFiles(
+					newSep := NewWriteNewBlobFiles(
 						testkeys.Comparer,
 						func() (objstorage.Writable, objstorage.ObjectMetadata, error) {
 							fn++
 							return objStore.Create(ctx, base.FileTypeBlob, fn, objstorage.CreateOptions{})
 						},
 						blob.FileWriterOptions{},
 						minimumSize,
-						valsep.WriteNewBlobFilesOptions{
+						WriteNewBlobFilesOptions{
 							ShortAttrExtractor: shortAttrExtractor,
 							InvalidValueCallback: func(userKey []byte, value []byte, err error) {
 								fmt.Fprintf(&buf, "# invalid value for key %q, value: %q: %s\n", userKey, value, err)
@@ -183,115 +180,11 @@ func TestValueSeparationPolicy(t *testing.T) {
 		})
 }
 
-func errShortAttrExtractor(key []byte, keyPrefixLen int, value []byte) (ShortAttribute, error) {
+func errShortAttrExtractor(
+	key []byte, keyPrefixLen int, value []byte,
+) (base.ShortAttribute, error) {
 	return 0, errors.New("short attribute extractor error")
 }
 
 // Assert that errShortAttrExtractor implements the ShortAttributeExtractor
-var _ ShortAttributeExtractor = errShortAttrExtractor
-
-// loggingRawWriter wraps a sstable.RawWriter and logs calls to Add and
-// AddWithBlobHandle to provide observability into the separation of values into
-// blob files.
-type loggingRawWriter struct {
-	w io.Writer
-	sstable.RawWriter
-}
-
-func (w *loggingRawWriter) Add(key InternalKey, value []byte, forceObsolete bool) error {
-	fmt.Fprintf(w.w, "RawWriter.Add(%q, %q, %t)\n", key, value, forceObsolete)
-	return w.RawWriter.Add(key, value, forceObsolete)
-}
-
-func (w *loggingRawWriter) AddWithBlobHandle(
-	key InternalKey, h blob.InlineHandle, attr base.ShortAttribute, forceObsolete bool,
-) error {
-	fmt.Fprintf(w.w, "RawWriter.AddWithBlobHandle(%q, %q, %x, %t)\n", key, h, attr, forceObsolete)
-	return w.RawWriter.AddWithBlobHandle(key, h, attr, forceObsolete)
-}
-
-// defineDBValueSeparator is a compact.ValueSeparation implementation used by
-// datadriven tests when defining a database state. It is a wrapper around
-// preserveBlobReferences that also parses string representations of blob
-// references from values.
-type defineDBValueSeparator struct {
-	bv    blobtest.Values
-	metas map[base.BlobFileID]*manifest.PhysicalBlobFile
-	pbr   valsep.ValueSeparation
-	kv    base.InternalKV
-}
-
-// Assert that *defineDBValueSeparator implements the compact.ValueSeparation interface.
-var _ valsep.ValueSeparation = (*defineDBValueSeparator)(nil)
-
-// SetNextOutputConfig implements the compact.ValueSeparation interface.
-func (vs *defineDBValueSeparator) SetNextOutputConfig(config valsep.ValueSeparationOutputConfig) {}
-
-// Kind implements the ValueSeparation interface.
-func (vs *defineDBValueSeparator) Kind() sstable.ValueSeparationKind {
-	return vs.pbr.Kind()
-}
-
-// MinimumSize implements the ValueSeparation interface.
-func (vs *defineDBValueSeparator) MinimumSize() int { return vs.pbr.MinimumSize() }
-
-// EstimatedFileSize returns an estimate of the disk space consumed by the current
-// blob file if it were closed now.
-func (vs *defineDBValueSeparator) EstimatedFileSize() uint64 {
-	return vs.pbr.EstimatedFileSize()
-}
-
-// EstimatedReferenceSize returns an estimate of the disk space consumed by the
-// current output sstable's blob references so far.
-func (vs *defineDBValueSeparator) EstimatedReferenceSize() uint64 {
-	return vs.pbr.EstimatedReferenceSize()
-}
-
-// Add adds the provided key-value pair to the sstable, possibly separating the
-// value into a blob file.
-func (vs *defineDBValueSeparator) Add(
-	tw sstable.RawWriter, kv *base.InternalKV, forceObsolete bool, _ bool,
-) error {
-	// In datadriven tests, all defined values are in-place initially. See
-	// runDBDefineCmdReuseFS.
-	v := kv.V.InPlaceValue()
-	// If the value doesn't begin with "blob", don't separate it.
-	if !bytes.HasPrefix(v, []byte("blob")) {
-		return tw.Add(kv.K, v, forceObsolete)
-	}
-
-	// This looks like a blob reference. Parse it.
-	iv, err := vs.bv.ParseInternalValue(string(v))
-	if err != nil {
-		return err
-	}
-	lv := iv.LazyValue()
-	// If we haven't seen this blob file before, fabricate a metadata for it.
-	fileID := lv.Fetcher.BlobFileID
-	meta, ok := vs.metas[fileID]
-	if !ok {
-		meta = &manifest.PhysicalBlobFile{
-			FileNum:      base.DiskFileNum(fileID),
-			CreationTime: uint64(time.Now().Unix()),
-		}
-		vs.metas[fileID] = meta
-	}
-	meta.Size += uint64(lv.Fetcher.Attribute.ValueLen)
-	meta.ValueSize += uint64(lv.Fetcher.Attribute.ValueLen)
-
-	// Return a KV that uses the original key but our constructed blob reference.
-	vs.kv.K = kv.K
-	vs.kv.V = iv
-	return vs.pbr.Add(tw, &vs.kv, forceObsolete, false /* isLikelyMVCCGarbage */)
-}
-
-// FinishOutput implements compact.ValueSeparation.
-func (d *defineDBValueSeparator) FinishOutput() (valsep.ValueSeparationMetadata, error) {
-	m, err := d.pbr.FinishOutput()
-	if err != nil {
-		return valsep.ValueSeparationMetadata{}, err
-	}
-	// TODO(jackson): Support setting a specific depth from the datadriven test.
-	m.BlobReferenceDepth = manifest.BlobReferenceDepth(len(m.BlobReferences))
-	return m, nil
-}
+var _ base.ShortAttributeExtractor = errShortAttrExtractor