Merge cockroachdb#139744

139744: storage: remove storage-engine flag r=RaduBerinde a=andrewbaptist The only storage engine for the past 4 years has been pebble. The storage engine is specfied at a node level but applies to the stores. If we want to add something like storage engine back in the future it would be done differently so its easiest to simply remove this for now. Epic: CRDB-41111 Release note (cli change): This change removes the ``--storage-engine`` parameter from the CLI. It has been deprecated for over 4 years. Co-authored-by: Andrew Baptist <[email protected]>
xinhaoz · Jan 28, 2025 · 712e9b2 · 712e9b2
2 parents 26def9e + e15a172
commit 712e9b2
Show file tree

Hide file tree

Showing 16 changed files with 13 additions and 124 deletions.
diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go
@@ -1092,11 +1092,8 @@ See the storage.wal_failover.unhealthy_op_threshold cluster setting.
 	}
 
 	StorageEngine = FlagInfo{
-		Name: "storage-engine",
-		Description: `
-Storage engine to use for all stores on this cockroach node. The only option is pebble. Deprecated;
-only present for backward compatibility.
-`,
+		Name:        "storage-engine",
+		Description: "Deprecated: only present for backward compatibility.",
 	}
 
 	SecondaryCache = FlagInfo{

diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go
@@ -292,6 +292,10 @@ func (f *keyTypeFilter) Set(v string) error {
 
 const backgroundEnvVar = "COCKROACH_BACKGROUND_RESTART"
 
+// This value is never read. It is used to hold the storage engine which is now
+// a hidden option.
+var deprecatedStorageEngine string
+
 func init() {
 	initCLIDefaults()
 
@@ -507,7 +511,11 @@ func init() {
 		cliflagcfg.StringFlag(f, &localityFile, cliflags.LocalityFile)
 
 		cliflagcfg.VarFlag(f, &storeSpecs, cliflags.Store)
-		cliflagcfg.VarFlag(f, &serverCfg.StorageEngine, cliflags.StorageEngine)
+
+		// deprecatedStorageEngine is only kept for backwards compatibility.
+		cliflagcfg.StringFlag(f, &deprecatedStorageEngine, cliflags.StorageEngine)
+		_ = pf.MarkHidden(cliflags.StorageEngine.Name)
+
 		cliflagcfg.VarFlag(f, &serverCfg.WALFailover, cliflags.WALFailover)
 		cliflagcfg.StringFlag(f, &serverCfg.SharedStorage, cliflags.SharedStorage)
 		cliflagcfg.VarFlag(f, &serverCfg.SecondaryCache, cliflags.SecondaryCache)

diff --git a/pkg/cli/start.go b/pkg/cli/start.go
@@ -38,7 +38,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/cgroups"
@@ -613,11 +612,6 @@ func runStartInternal(
 		return err
 	}
 
-	// Configure the default storage engine.
-	if serverCfg.StorageEngine == enginepb.EngineTypeDefault {
-		serverCfg.StorageEngine = enginepb.EngineTypePebble
-	}
-
 	// The configuration is now ready to report to the user and the log
 	// file. We had to wait after InitNode() so that all configuration
 	// environment variables, which are reported too, have been read and
@@ -1241,7 +1235,6 @@ func reportServerInfo(
 	for i, spec := range serverCfg.Stores.Specs {
 		buf.Printf("store[%d]:\t%s\n", i, log.SafeManaged(spec))
 	}
-	buf.Printf("storage engine: \t%s\n", &serverCfg.StorageEngine)
 
 	// Print the commong server identifiers.
 	if baseCfg.ClusterName != "" {

diff --git a/pkg/protos.bzl b/pkg/protos.bzl
@@ -131,7 +131,6 @@ PROTO_FILES = [
     "//pkg/sql/sessiondatapb:session_revival_token.proto",
     "//pkg/sql/sqlstats/insights:insights.proto",
     "//pkg/sql/types:types.proto",
-    "//pkg/storage/enginepb:engine.proto",
     "//pkg/storage/enginepb:file_registry.proto",
     "//pkg/storage/enginepb:mvcc.proto",
     "//pkg/storage/enginepb:mvcc3.proto",

diff --git a/pkg/server/config.go b/pkg/server/config.go
@@ -34,7 +34,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/disk"
-	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"github.com/cockroachdb/cockroach/pkg/ts"
 	"github.com/cockroachdb/cockroach/pkg/util"
@@ -195,10 +194,6 @@ type BaseConfig struct {
 	// Locality is a description of the topography of the server.
 	Locality roachpb.Locality
 
-	// StorageEngine specifies the engine type (eg. rocksdb, pebble) to use to
-	// instantiate stores.
-	StorageEngine enginepb.EngineType
-
 	// TestingKnobs is used for internal test controls only.
 	TestingKnobs base.TestingKnobs
 
@@ -319,7 +314,6 @@ func (cfg *BaseConfig) SetDefaults(
 	cfg.MaxOffset = MaxOffsetType(base.DefaultMaxClockOffset)
 	cfg.DisableMaxOffsetCheck = false
 	cfg.DefaultZoneConfig = zonepb.DefaultZoneConfig()
-	cfg.StorageEngine = storage.DefaultStorageEngine
 	cfg.WALFailover = base.WALFailoverConfig{Mode: base.WALFailoverDefault}
 	cfg.TestingInsecureWebAccess = disableWebLogin
 	cfg.Stores = base.StoreSpecList{

diff --git a/pkg/server/node.go b/pkg/server/node.go
@@ -828,12 +828,6 @@ func (n *Node) start(
 		}
 	})
 
-	allEngines := append([]storage.Engine(nil), state.initializedEngines...)
-	allEngines = append(allEngines, state.uninitializedEngines...)
-	for _, e := range allEngines {
-		t := e.Type()
-		log.Infof(ctx, "started with engine type %v", &t)
-	}
 	log.Infof(ctx, "started with attributes %v", attrs.Attrs)
 
 	n.startPeriodicLivenessCompaction(n.stopper, livenessRangeCompactInterval)

diff --git a/pkg/server/server.go b/pkg/server/server.go
@@ -103,7 +103,6 @@ import (
 	_ "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttljob"      // register jobs declared outside of pkg/sql
 	_ "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttlschedule" // register schedules declared outside of pkg/sql
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/ts"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/admission"
@@ -1851,7 +1850,6 @@ func (s *topLevelServer) PreStart(ctx context.Context) error {
 			"cluster":         s.StorageClusterID().String(),
 			"node":            s.NodeID().String(),
 			"server_id":       fmt.Sprintf("%s-%s", s.StorageClusterID().Short(), s.NodeID()),
-			"engine_type":     s.cfg.StorageEngine.String(),
 			"encrypted_store": strconv.FormatBool(encryptedStore),
 		})
 	})
@@ -2002,13 +2000,7 @@ func (s *topLevelServer) PreStart(ctx context.Context) error {
 
 	// Record node start in telemetry. Get the right counter for this storage
 	// engine type as well as type of start (initial boot vs restart).
-	nodeStartCounter := "storage.engine."
-	switch s.cfg.StorageEngine {
-	case enginepb.EngineTypeDefault:
-		fallthrough
-	case enginepb.EngineTypePebble:
-		nodeStartCounter += "pebble."
-	}
+	nodeStartCounter := "storage.engine.pebble."
 	if s.InitialStart() {
 		nodeStartCounter += "initial-boot"
 	} else {

diff --git a/pkg/server/server_controller_new_server.go b/pkg/server/server_controller_new_server.go
@@ -229,7 +229,6 @@ func makeSharedProcessTenantServerConfig(
 	baseCfg.Config.DisableClusterNameVerification = kvServerCfg.Config.DisableClusterNameVerification
 
 	baseCfg.MaxOffset = kvServerCfg.BaseConfig.MaxOffset
-	baseCfg.StorageEngine = kvServerCfg.BaseConfig.StorageEngine
 	baseCfg.TestingInsecureWebAccess = kvServerCfg.BaseConfig.TestingInsecureWebAccess
 	baseCfg.Locality = kvServerCfg.BaseConfig.Locality
 	baseCfg.EnableDemoLoginEndpoint = kvServerCfg.BaseConfig.EnableDemoLoginEndpoint

diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go
@@ -712,7 +712,6 @@ func (s *SQLServerWrapper) PreStart(ctx context.Context) error {
 	// to add this information. See below.
 	sentry.ConfigureScope(func(scope *sentry.Scope) {
 		scope.SetTags(map[string]string{
-			"engine_type":     s.sqlServer.cfg.StorageEngine.String(),
 			"encrypted_store": strconv.FormatBool(encryptedStore),
 		})
 	})

diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go
@@ -96,8 +96,6 @@ func makeTestBaseConfig(st *cluster.Settings, tr *tracing.Tracer) BaseConfig {
 	baseCfg := MakeBaseConfig(st, tr, base.DefaultTestStoreSpec)
 	// Test servers start in secure mode by default.
 	baseCfg.Insecure = false
-	// Configure test storage engine.
-	baseCfg.StorageEngine = storage.DefaultStorageEngine
 	// Load test certs. In addition, the tests requiring certs
 	// need to call securityassets.SetLoader(securitytest.EmbeddedAssets)
 	// in their init to mock out the file system calls for calls to AssetFS,

diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go
@@ -18,7 +18,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"github.com/cockroachdb/cockroach/pkg/storage/pebbleiter"
-	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -33,13 +32,6 @@ import (
 	prometheusgo "github.com/prometheus/client_model/go"
 )
 
-// DefaultStorageEngine represents the default storage engine to use.
-var DefaultStorageEngine enginepb.EngineType
-
-func init() {
-	_ = DefaultStorageEngine.Set(envutil.EnvOrDefaultString("COCKROACH_STORAGE_ENGINE", "pebble"))
-}
-
 // SimpleMVCCIterator is an interface for iterating over key/value pairs in an
 // engine. SimpleMVCCIterator implementations are thread safe unless otherwise
 // noted. SimpleMVCCIterator is a subset of the functionality offered by
@@ -1019,8 +1011,6 @@ type Engine interface {
 	// in the passed-in keyRanges; reads are not guaranteed to be consistent
 	// outside of these bounds.
 	NewEventuallyFileOnlySnapshot(keyRanges []roachpb.Span) EventuallyFileOnlyReader
-	// Type returns engine type.
-	Type() enginepb.EngineType
 	// IngestLocalFiles atomically links a slice of files into the RocksDB
 	// log-structured merge-tree.
 	IngestLocalFiles(ctx context.Context, paths []string) error

diff --git a/pkg/storage/enginepb/BUILD.bazel b/pkg/storage/enginepb/BUILD.bazel
@@ -5,7 +5,6 @@ load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 proto_library(
     name = "enginepb_proto",
     srcs = [
-        "engine.proto",
         "file_registry.proto",
         "mvcc.proto",
         "mvcc3.proto",
@@ -38,7 +37,6 @@ go_library(
     name = "enginepb",
     srcs = [
         "decode.go",
-        "engine.go",
         "file_registry.go",
         "mvcc.go",
         "mvcc3.go",

diff --git a/pkg/storage/enginepb/engine.go b/pkg/storage/enginepb/engine.go
diff --git a/pkg/storage/enginepb/engine.proto b/pkg/storage/enginepb/engine.proto
diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go
@@ -2037,11 +2037,6 @@ func (p *Pebble) NewEventuallyFileOnlySnapshot(keyRanges []roachpb.Span) Eventua
 	}
 }
 
-// Type implements the Engine interface.
-func (p *Pebble) Type() enginepb.EngineType {
-	return enginepb.EngineTypePebble
-}
-
 // IngestLocalFiles implements the Engine interface.
 func (p *Pebble) IngestLocalFiles(ctx context.Context, paths []string) error {
 	return p.db.Ingest(ctx, paths)

diff --git a/pkg/util/log/logcrash/crash_reporting_packet_test.go b/pkg/util/log/logcrash/crash_reporting_packet_test.go
@@ -121,7 +121,7 @@ func TestCrashReportingPacket(t *testing.T) {
 		}(),
 			regexp.MustCompile(`crash_reporting_packet_test.go:\d+: panic: boom`),
 		},
-		{regexp.MustCompile(`^[a-z0-9]{8}-1$`), 14, func() string {
+		{regexp.MustCompile(`^[a-z0-9]{8}-1$`), 13, func() string {
 			message := prefix
 			// gccgo stack traces are different in the presence of function literals.
 			if runtime.Compiler == "gccgo" {