Repository: cockroachdb/pebble (go)

Issue:
l0_sublevels: unskip TestL0Sublevels_LargeImportL0
This test has been skipped for years. It needs to be fixed and un-skipped, or removed if equivalent test coverage is already provided by other means.



Pull request:
internal/manifest: delete L0Sublevels_LargeImport test
Previously we had skipped the L0Sublevels_LargeImport test as
it was not well-written and buggy. On closer inspection it does
not provide any net new test coverage and is just a sanity test
with loading a large manifest and picking some compactions in a row,
something that will happen with metamorphic test runs too. Plus,
the same manifest is already loaded in some benchmarks. Fixing
the test added 60+ seconds to the runtime of the whole test suite.

This change deletes this test.

Fixes #2710 

Patch:
diff --git a/internal/errorfs/errorfs.go b/internal/errorfs/errorfs.go
index 86ac8e5d..9ebc45a9 100644
--- a/internal/errorfs/errorfs.go
+++ b/internal/errorfs/errorfs.go
@@ -229,7 +229,7 @@ func (fs *FS) OpenReadWrite(name string, opts ...vfs.OpenOption) (vfs.File, erro
 	if err := fs.inj.MaybeError(OpOpen, name); err != nil {
 		return nil, err
 	}
-	f, err := fs.fs.Open(name)
+	f, err := fs.fs.OpenReadWrite(name)
 	if err != nil {
 		return nil, err
 	}
diff --git a/metamorphic/ops.go b/metamorphic/ops.go
index eb4d12e8..27e43571 100644
--- a/metamorphic/ops.go
+++ b/metamorphic/ops.go
@@ -102,6 +102,16 @@ type checkpointOp struct {
 }
 
 func (o *checkpointOp) run(t *test, h historyRecorder) {
+	// TODO(josh): db.Checkpoint does not work with shared storage yet.
+	// It would be better to filter out ahead of calling run on the op,
+	// by setting the weight that generator.go uses to zero, or similar.
+	// But IIUC the ops are shared for ALL the metamorphic test runs, so
+	// not sure how to do that easily:
+	// https://github.com/cockroachdb/pebble/blob/master/metamorphic/meta.go#L177
+	if t.testOpts.sharedStorageEnabled {
+		h.Recordf("%s // %v", o, nil)
+		return
+	}
 	var opts []pebble.CheckpointOption
 	if len(o.spans) > 0 {
 		opts = append(opts, pebble.WithRestrictToSpans(o.spans))
@@ -1209,6 +1219,11 @@ func (o *dbRatchetFormatMajorVersionOp) syncObjs() objIDSlice { return nil }
 type dbRestartOp struct{}
 
 func (o *dbRestartOp) run(t *test, h historyRecorder) {
+	// TODO(josh): db.Restart does not work with shared storage yet.
+	if t.testOpts.sharedStorageEnabled {
+		h.Recordf("%s", o)
+		return
+	}
 	if err := t.restartDB(); err != nil {
 		h.Recordf("%s // %v", o, err)
 		h.history.err.Store(errors.Wrap(err, "dbRestartOp"))
diff --git a/metamorphic/options.go b/metamorphic/options.go
index f21ec5e3..d5e8d8d3 100644
--- a/metamorphic/options.go
+++ b/metamorphic/options.go
@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/pebble/bloom"
 	"github.com/cockroachdb/pebble/internal/cache"
 	"github.com/cockroachdb/pebble/internal/testkeys"
+	"github.com/cockroachdb/pebble/objstorage/shared"
 	"github.com/cockroachdb/pebble/sstable"
 	"github.com/cockroachdb/pebble/vfs"
 	"golang.org/x/exp/rand"
@@ -91,6 +92,17 @@ func parseOptions(
 			case "TestOptions.async_apply_to_db":
 				opts.asyncApplyToDB = true
 				return true
+			case "TestOptions.shared_storage_enabled":
+				opts.sharedStorageEnabled = true
+				opts.Opts.Experimental.SharedStorage = shared.MakeSimpleFactory(map[shared.Locator]shared.Storage{
+					"": shared.NewInMem(),
+				})
+				opts.Opts.Experimental.CreateOnShared = true
+				return true
+			case "TestOptions.secondary_cache_enabled":
+				opts.secondaryCacheEnabled = true
+				opts.Opts.Experimental.SecondaryCacheSize = 1024 * 1024 * 32 // 32 MBs
+				return true
 			default:
 				if customOptionParsers == nil {
 					return false
@@ -145,6 +157,12 @@ func optionsToString(opts *TestOptions) string {
 	if opts.asyncApplyToDB {
 		fmt.Fprint(&buf, "  async_apply_to_db=true\n")
 	}
+	if opts.sharedStorageEnabled {
+		fmt.Fprint(&buf, "  shared_storage_enabled=true\n")
+	}
+	if opts.secondaryCacheEnabled {
+		fmt.Fprint(&buf, "  secondary_cache_enabled=true\n")
+	}
 	for _, customOpt := range opts.CustomOpts {
 		fmt.Fprintf(&buf, "  %s=%s\n", customOpt.Name(), customOpt.Value())
 	}
@@ -208,6 +226,11 @@ type TestOptions struct {
 	enableValueBlocks bool
 	// Use DB.ApplyNoSyncWait for applies that want to sync the WAL.
 	asyncApplyToDB bool
+	// Enable the use of shared storage.
+	sharedStorageEnabled bool
+	// Enable the secondary cache. Only effective if sharedStorageEnabled is
+	// also true.
+	secondaryCacheEnabled bool
 }
 
 // CustomOption defines a custom option that configures the behavior of an
@@ -349,6 +372,11 @@ func standardOptions() []*TestOptions {
 [Options]
   format_major_version=%s
 `, newestFormatMajorVersionTODO),
+		27: `
+[TestOptions]
+  shared_storage_enabled=true
+  secondary_cache_enabled=true
+`,
 	}
 
 	opts := make([]*TestOptions, len(stdOpts))
@@ -469,6 +497,20 @@ func randomOptions(
 		testOpts.Opts.Experimental.EnableValueBlocks = func() bool { return true }
 	}
 	testOpts.asyncApplyToDB = rng.Intn(2) != 0
+	// 20% of time, enable shared storage.
+	if rng.Intn(5) == 0 {
+		testOpts.sharedStorageEnabled = true
+		testOpts.Opts.Experimental.SharedStorage = shared.MakeSimpleFactory(map[shared.Locator]shared.Storage{
+			"": shared.NewInMem(),
+		})
+		testOpts.Opts.Experimental.CreateOnShared = true
+		// If shared storage is enabled, enable secondary cache 50% of time.
+		if rng.Intn(2) == 0 {
+			testOpts.secondaryCacheEnabled = true
+			// TODO(josh): Randomize various secondary cache settings.
+			testOpts.Opts.Experimental.SecondaryCacheSize = 1024 * 1024 * 32 // 32 MBs
+		}
+	}
 	return testOpts
 }
 
diff --git a/open.go b/open.go
index 602cab11..2cb489a8 100644
--- a/open.go
+++ b/open.go
@@ -298,6 +298,7 @@ func Open(dirname string, opts *Options) (db *DB, _ error) {
 	providerSettings.Shared.StorageFactory = opts.Experimental.SharedStorage
 	providerSettings.Shared.CreateOnShared = opts.Experimental.CreateOnShared
 	providerSettings.Shared.CreateOnSharedLocator = opts.Experimental.CreateOnSharedLocator
+	providerSettings.Shared.CacheSizeBytes = opts.Experimental.SecondaryCacheSize
 
 	d.objProvider, err = objstorageprovider.Open(providerSettings)
 	if err != nil {
diff --git a/options.go b/options.go
index 9266cf7c..1ad8bc61 100644
--- a/options.go
+++ b/options.go
@@ -667,6 +667,10 @@ type Options struct {
 		// using CreateOnSharedLocator. Can only be used when SharedStorage is set.
 		CreateOnShared        bool
 		CreateOnSharedLocator shared.Locator
+
+		// CacheSizeBytes is the size of the on-disk block cache for objects
+		// on shared storage. If it is 0, no cache is used.
+		SecondaryCacheSize int64
 	}
 
 	// Filters is a map from filter policy name to filter policy. It is used for
diff --git a/vfs/disk_health.go b/vfs/disk_health.go
index f456d505..9ce7b712 100644
--- a/vfs/disk_health.go
+++ b/vfs/disk_health.go
@@ -704,7 +704,7 @@ func (d *diskHealthCheckingFS) Open(name string, opts ...OpenOption) (File, erro
 
 // OpenReadWrite implements the FS interface.
 func (d *diskHealthCheckingFS) OpenReadWrite(name string, opts ...OpenOption) (File, error) {
-	return d.fs.Open(name, opts...)
+	return d.fs.OpenReadWrite(name, opts...)
 }
 
 // OpenDir implements the FS interface.


Test patch:
diff --git a/internal/manifest/l0_sublevels_test.go b/internal/manifest/l0_sublevels_test.go
index 0836ea70..a9e929ba 100644
--- a/internal/manifest/l0_sublevels_test.go
+++ b/internal/manifest/l0_sublevels_test.go
@@ -57,56 +57,6 @@ func readManifest(filename string) (*Version, error) {
 	return v, nil
 }
 
-func TestL0Sublevels_LargeImportL0(t *testing.T) {
-	t.Skip("#2710")
-	v, err := readManifest("testdata/MANIFEST_import")
-	require.NoError(t, err)
-
-	sublevels, err := NewL0Sublevels(&v.Levels[0],
-		base.DefaultComparer.Compare, base.DefaultFormatter, 5<<20)
-	require.NoError(t, err)
-	fmt.Printf("L0Sublevels:\n%s\n\n", sublevels)
-
-	for i := 0; ; i++ {
-		c, err := sublevels.PickBaseCompaction(2, LevelSlice{})
-		require.NoError(t, err)
-		if c == nil {
-			break
-		}
-		fmt.Printf("%d: base compaction: filecount: %d, bytes: %d, interval: [%d, %d], seed depth: %d\n",
-			i, len(c.Files), c.fileBytes, c.minIntervalIndex, c.maxIntervalIndex, c.seedIntervalStackDepthReduction)
-		var files []*FileMetadata
-		for i := range c.Files {
-			if c.FilesIncluded[i] {
-				c.Files[i].CompactionState = CompactionStateCompacting
-				files = append(files, c.Files[i])
-			}
-		}
-		require.NoError(t, sublevels.UpdateStateForStartedCompaction(
-			[]LevelSlice{NewLevelSliceSeqSorted(files)}, true))
-	}
-
-	for i := 0; ; i++ {
-		c, err := sublevels.PickIntraL0Compaction(math.MaxUint64, 2)
-		require.NoError(t, err)
-		if c == nil {
-			break
-		}
-		fmt.Printf("%d: intra-L0 compaction: filecount: %d, bytes: %d, interval: [%d, %d], seed depth: %d\n",
-			i, len(c.Files), c.fileBytes, c.minIntervalIndex, c.maxIntervalIndex, c.seedIntervalStackDepthReduction)
-		var files []*FileMetadata
-		for i := range c.Files {
-			if c.FilesIncluded[i] {
-				c.Files[i].CompactionState = CompactionStateCompacting
-				c.Files[i].IsIntraL0Compacting = true
-				files = append(files, c.Files[i])
-			}
-		}
-		require.NoError(t, sublevels.UpdateStateForStartedCompaction(
-			[]LevelSlice{NewLevelSliceSeqSorted(files)}, false))
-	}
-}
-
 func visualizeSublevels(
 	s *L0Sublevels, compactionFiles bitSet, otherLevels [][]*FileMetadata,
 ) string {
diff --git a/metamorphic/options_test.go b/metamorphic/options_test.go
index b5c569d9..df274d1c 100644
--- a/metamorphic/options_test.go
+++ b/metamorphic/options_test.go
@@ -66,6 +66,7 @@ func TestOptionsRoundtrip(t *testing.T) {
 		"MaxConcurrentCompactions:",
 		"Experimental.EnableValueBlocks:",
 		"Experimental.DisableIngestAsFlushable:",
+		"Experimental.SharedStorage:",
 		// Floating points
 		"Experimental.PointTombstoneWeight:",
 	}
diff --git a/metamorphic/test.go b/metamorphic/test.go
index ecd23225..af30fe21 100644
--- a/metamorphic/test.go
+++ b/metamorphic/test.go
@@ -138,6 +138,16 @@ func (t *test) init(h *history, dir string, testOpts *TestOptions) error {
 	}
 	h.log.Printf("// db.Open() %v", err)
 
+	if t.testOpts.sharedStorageEnabled {
+		err = withRetries(func() error {
+			return db.SetCreatorID(1)
+		})
+		if err != nil {
+			return err
+		}
+		h.log.Printf("// db.SetCreatorID() %v", err)
+	}
+
 	t.tmpDir = t.opts.FS.PathJoin(dir, "tmp")
 	if err = t.opts.FS.MkdirAll(t.tmpDir, 0755); err != nil {
 		return err