Merge #158161

158161: storage: remove Engine.PreIngestDelay r=jbowens a=jbowens

Previously the Engine exposed a method that would block a variable amount of time depending on the health of the LSM. This was a rudimentary backpressure mechanism that predated admission control (and quorum replication flow control). We no longer require this mechanism and can remove it and the corresponding cluster settings.

Release note: none
Epic: none

Co-authored-by: Jackson Owens <[email protected]>

Author: craig[bot]

docs/generated/metrics/metrics.yaml

@@ -10598,14 +10598,6 @@ layers:
       unit: COUNT
       aggregation: AVG
       derivative: NON_NEGATIVE_DERIVATIVE
-    - name: addsstable.delay.enginebackpressure
-      exported_name: addsstable_delay_enginebackpressure
-      description: Amount by which evaluation of AddSSTable requests was delayed by storage-engine backpressure
-      y_axis_label: Nanoseconds
-      type: COUNTER
-      unit: NANOSECONDS
-      aggregation: AVG
-      derivative: NON_NEGATIVE_DERIVATIVE
     - name: addsstable.delay.total
       exported_name: addsstable_delay_total
       description: Amount by which evaluation of AddSSTable requests was delayed

pkg/kv/kvserver/metrics.go

@@ -2621,12 +2621,6 @@ throttled they do count towards 'delay.total' and 'delay.enginebackpressure'.
 		Measurement: "Nanoseconds",
 		Unit:        metric.Unit_NANOSECONDS,
 	}
-	metaAddSSTableEvalEngineDelay = metric.Metadata{
-		Name:        "addsstable.delay.enginebackpressure",
-		Help:        "Amount by which evaluation of AddSSTable requests was delayed by storage-engine backpressure",
-		Measurement: "Nanoseconds",
-		Unit:        metric.Unit_NANOSECONDS,
-	}
 
 	// Export request counter.
 	metaExportEvalTotalDelay = metric.Metadata{
@@ -3402,12 +3396,11 @@ type StoreMetrics struct {
 
 	// AddSSTable stats: how many AddSSTable commands were proposed and how many
 	// were applied? How many applications required writing a copy?
-	AddSSTableProposals           *metric.Counter
-	AddSSTableApplications        *metric.Counter
-	AddSSTableApplicationCopies   *metric.Counter
-	AddSSTableAsWrites            *metric.Counter
-	AddSSTableProposalTotalDelay  *metric.Counter
-	AddSSTableProposalEngineDelay *metric.Counter
+	AddSSTableProposals          *metric.Counter
+	AddSSTableApplications       *metric.Counter
+	AddSSTableApplicationCopies  *metric.Counter
+	AddSSTableAsWrites           *metric.Counter
+	AddSSTableProposalTotalDelay *metric.Counter
 
 	// Export request stats.
 	ExportRequestProposalTotalDelay *metric.Counter
@@ -4214,12 +4207,11 @@ func newStoreMetrics(histogramWindow time.Duration) *StoreMetrics {
 		BackpressuredOnSplitRequests: metric.NewGauge(metaBackpressuredOnSplitRequests),
 
 		// AddSSTable proposal + applications counters.
-		AddSSTableProposals:           metric.NewCounter(metaAddSSTableProposals),
-		AddSSTableApplications:        metric.NewCounter(metaAddSSTableApplications),
-		AddSSTableAsWrites:            metric.NewCounter(metaAddSSTableAsWrites),
-		AddSSTableApplicationCopies:   metric.NewCounter(metaAddSSTableApplicationCopies),
-		AddSSTableProposalTotalDelay:  metric.NewCounter(metaAddSSTableEvalTotalDelay),
-		AddSSTableProposalEngineDelay: metric.NewCounter(metaAddSSTableEvalEngineDelay),
+		AddSSTableProposals:          metric.NewCounter(metaAddSSTableProposals),
+		AddSSTableApplications:       metric.NewCounter(metaAddSSTableApplications),
+		AddSSTableAsWrites:           metric.NewCounter(metaAddSSTableAsWrites),
+		AddSSTableApplicationCopies:  metric.NewCounter(metaAddSSTableApplicationCopies),
+		AddSSTableProposalTotalDelay: metric.NewCounter(metaAddSSTableEvalTotalDelay),
 
 		// ExportRequest proposal.
 		ExportRequestProposalTotalDelay: metric.NewCounter(metaExportEvalTotalDelay),

pkg/kv/kvserver/store_send.go

@@ -330,18 +330,11 @@ func (s *Store) maybeThrottleBatch(
 		if err != nil {
 			return nil, err
 		}
+		waited := timeutil.Since(before)
 
-		beforeEngineDelay := timeutil.Now()
-		// TODO(sep-raft-log): can we get rid of this?
-		s.TODOEngine().PreIngestDelay(ctx)
-		after := timeutil.Now()
-
-		waited, waitedEngine := after.Sub(before), after.Sub(beforeEngineDelay)
 		s.metrics.AddSSTableProposalTotalDelay.Inc(waited.Nanoseconds())
-		s.metrics.AddSSTableProposalEngineDelay.Inc(waitedEngine.Nanoseconds())
 		if waited > time.Second {
-			log.KvExec.Infof(ctx, "SST ingestion was delayed by %v (%v for storage engine back-pressure)",
-				waited, waitedEngine)
+			log.KvExec.Infof(ctx, "SST ingestion was delayed by %v", waited)
 		}
 		return res, nil
 

pkg/roachprod/agents/opentelemetry/cockroachdb_metrics.go

@@ -34,7 +34,6 @@ var cockroachdbMetrics = map[string]string{
     "addsstable_applications": "addsstable.applications",
     "addsstable_aswrites": "addsstable.aswrites",
     "addsstable_copies": "addsstable.copies",
-    "addsstable_delay_enginebackpressure": "addsstable.delay.enginebackpressure",
     "addsstable_delay_total": "addsstable.delay.total",
     "addsstable_proposals": "addsstable.proposals",
     "admission_admitted_elastic_cpu": "admission.admitted.elastic_cpu",

pkg/settings/registry.go

@@ -276,7 +276,10 @@ var retiredSettings = map[InternalKey]struct{}{
 	"storage.columnar_blocks.enabled": {},
 
 	// removed as of 26.1
-	"sql.distsql_planning.use_gossip.enabled": {},
+	"sql.distsql_planning.use_gossip.enabled":             {},
+	"rocksdb.ingest_backpressure.l0_file_count_threshold": {},
+	"rocksdb.ingest_backpressure.max_delay":               {},
+	"pebble.pre_ingest_delay.enabled":                     {},
 }
 
 // grandfatheredDefaultSettings is the list of "grandfathered" existing sql.defaults

pkg/storage/engine.go

@@ -13,8 +13,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/settings"
-	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"github.com/cockroachdb/cockroach/pkg/storage/pebbleiter"
@@ -1053,10 +1051,6 @@ type Engine interface {
 	// modified or deleted by the Engine doing the ingestion.
 	IngestExternalFiles(ctx context.Context, external []pebble.ExternalFile) (pebble.IngestOperationStats, error)
 
-	// PreIngestDelay offers an engine the chance to backpressure ingestions.
-	// When called, it may choose to block if the engine determines that it is in
-	// or approaching a state where further ingestions may risk its health.
-	PreIngestDelay(ctx context.Context)
 	// ApproximateDiskBytes returns an approximation of the on-disk size and file
 	// counts for the given key span, along with how many of those bytes are on
 	// remote, as well as specifically external remote, storage.
@@ -1718,79 +1712,6 @@ func ClearRangeWithHeuristic(
 	return nil
 }
 
-var ingestDelayL0Threshold = settings.RegisterIntSetting(
-	settings.ApplicationLevel,
-	"rocksdb.ingest_backpressure.l0_file_count_threshold",
-	"number of L0 files after which to backpressure SST ingestions",
-	20,
-)
-
-var ingestDelayTime = settings.RegisterDurationSetting(
-	settings.ApplicationLevel,
-	"rocksdb.ingest_backpressure.max_delay",
-	"maximum amount of time to backpressure a single SST ingestion",
-	time.Second*5,
-)
-
-var preIngestDelayEnabled = settings.RegisterBoolSetting(
-	settings.SystemOnly,
-	"pebble.pre_ingest_delay.enabled",
-	"controls whether the pre-ingest delay mechanism is active",
-	false,
-)
-
-// PreIngestDelay may choose to block for some duration if L0 has an excessive
-// number of files in it or if PendingCompactionBytesEstimate is elevated. This
-// it is intended to be called before ingesting a new SST, since we'd rather
-// backpressure the bulk operation adding SSTs than slow down the whole RocksDB
-// instance and impact all foreground traffic by adding too many files to it.
-// After the number of L0 files exceeds the configured limit, it gradually
-// begins delaying more for each additional file in L0 over the limit until
-// hitting its configured (via settings) maximum delay. If the pending
-// compaction limit is exceeded, it waits for the maximum delay.
-func preIngestDelay(ctx context.Context, eng Engine, settings *cluster.Settings) {
-	if settings == nil {
-		return
-	}
-	if !preIngestDelayEnabled.Get(&settings.SV) {
-		return
-	}
-	metrics := eng.GetMetrics()
-	targetDelay := calculatePreIngestDelay(settings, metrics.Metrics)
-
-	if targetDelay == 0 {
-		return
-	}
-	log.VEventf(ctx, 2, "delaying SST ingestion %s. %d L0 files, %d L0 Sublevels",
-		targetDelay, metrics.Levels[0].Tables.Count, metrics.Levels[0].Sublevels)
-
-	select {
-	case <-time.After(targetDelay):
-	case <-ctx.Done():
-	}
-}
-
-func calculatePreIngestDelay(settings *cluster.Settings, metrics *pebble.Metrics) time.Duration {
-	maxDelay := ingestDelayTime.Get(&settings.SV)
-	l0ReadAmpLimit := ingestDelayL0Threshold.Get(&settings.SV)
-
-	const ramp = 10
-	l0ReadAmp := int64(metrics.Levels[0].Tables.Count)
-	if metrics.Levels[0].Sublevels >= 0 {
-		l0ReadAmp = int64(metrics.Levels[0].Sublevels)
-	}
-
-	if l0ReadAmp > l0ReadAmpLimit {
-		delayPerFile := maxDelay / time.Duration(ramp)
-		targetDelay := time.Duration(l0ReadAmp-l0ReadAmpLimit) * delayPerFile
-		if targetDelay > maxDelay {
-			return maxDelay
-		}
-		return targetDelay
-	}
-	return 0
-}
-
 // Helper function to implement Reader.MVCCIterate().
 func iterateOnReader(
 	ctx context.Context,

pkg/storage/engine_test.go

@@ -20,7 +20,6 @@ import (
 	"strconv"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
@@ -1186,34 +1185,6 @@ func TestCreateCheckpoint_SpanConstrained(t *testing.T) {
 	}
 }
 
-func TestIngestDelayLimit(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-	s := cluster.MakeTestingClusterSettings()
-
-	max, ramp := time.Second*5, time.Second*5/10
-
-	for _, tc := range []struct {
-		exp           time.Duration
-		fileCount     uint64
-		sublevelCount int32
-	}{
-		{0, 0, 0},
-		{0, 19, -1},
-		{0, 20, -1},
-		{ramp, 21, -1},
-		{ramp * 2, 22, -1},
-		{ramp * 2, 22, 22},
-		{ramp * 2, 55, 22},
-		{max, 55, -1},
-	} {
-		var m pebble.Metrics
-		m.Levels[0].Tables.Count = tc.fileCount
-		m.Levels[0].Sublevels = tc.sublevelCount
-		require.Equal(t, tc.exp, calculatePreIngestDelay(s, &m))
-	}
-}
-
 func TestEngineFS(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/storage/pebble.go

@@ -2255,11 +2255,6 @@ func (p *Pebble) IngestExternalFiles(
 	return p.db.IngestExternalFiles(ctx, external)
 }
 
-// PreIngestDelay implements the Engine interface.
-func (p *Pebble) PreIngestDelay(ctx context.Context) {
-	preIngestDelay(ctx, p, p.cfg.settings)
-}
-
 // GetTableMetrics implements the Engine interface.
 func (p *Pebble) GetTableMetrics(start, end roachpb.Key) ([]enginepb.SSTableMetricsInfo, error) {
 	filterOpt := pebble.WithKeyRangeFilter(