Merge cockroachdb#93323

93323: storage: remove time-based ExportRequest resource limiter r=sumeerbhola a=stevendanna This resource limiter was disabled by default and competes with the enabled-by-default elastic CPU limiter. Removing it simplifies the code a bit with very little downside. Epic: None Release note: None Co-authored-by: Steven Danna <[email protected]>
DrewKimball · Dec 20, 2022 · f9d7473 · f9d7473
2 parents 060cdcb + e843b64
commit f9d7473
Show file tree

Hide file tree

Showing 9 changed files with 1 addition and 403 deletions.
diff --git a/pkg/gen/stringer.bzl b/pkg/gen/stringer.bzl
@@ -44,7 +44,6 @@ STRINGER_SRCS = [
     "//pkg/sql:nodestatus_string.go",
     "//pkg/sql:txneventtype_string.go",
     "//pkg/sql:txntype_string.go",
-    "//pkg/storage:resourcelimitreached_string.go",
     "//pkg/util/encoding:type_string.go",
     "//pkg/util/timeutil/pgdate:field_string.go",
     "//pkg/workload/schemachange:optype_string.go",

diff --git a/pkg/kv/kvserver/batcheval/BUILD.bazel b/pkg/kv/kvserver/batcheval/BUILD.bazel
@@ -83,7 +83,6 @@ go_library(
         "//pkg/util/log",
         "//pkg/util/mon",
         "//pkg/util/protoutil",
-        "//pkg/util/timeutil",
         "//pkg/util/tracing",
         "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",

diff --git a/pkg/kv/kvserver/batcheval/cmd_export.go b/pkg/kv/kvserver/batcheval/cmd_export.go
@@ -23,7 +23,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
 )
@@ -62,21 +61,6 @@ var ExportRequestMaxAllowedFileSizeOverage = settings.RegisterByteSizeSetting(
 	64<<20, /* 64 MiB */
 ).WithPublic()
 
-// exportRequestMaxIterationTime controls time spent by export request iterating
-// over data in underlying storage. This threshold preventing export request from
-// holding locks for too long and preventing non mvcc operations from progressing.
-// If request takes longer than this threshold it would stop and return already
-// collected data and allow caller to use resume span to continue.
-var exportRequestMaxIterationTime = settings.RegisterDurationSetting(
-	settings.TenantWritable,
-	"kv.bulk_sst.max_request_time",
-	"if set, limits amount of time spent in export requests; "+
-		"if export request can not finish within allocated time it will resume from the point it stopped in "+
-		"subsequent request",
-	// Feature is disabled by default.
-	0,
-)
-
 func init() {
 	RegisterReadOnlyCommand(roachpb.Export, declareKeysExport, evalExport)
 }
@@ -165,8 +149,6 @@ func evalExport(
 		maxSize = targetSize + uint64(allowedOverage)
 	}
 
-	maxRunTime := exportRequestMaxIterationTime.Get(&cArgs.EvalCtx.ClusterSettings().SV)
-
 	var maxIntents uint64
 	if m := storage.MaxIntentsPerWriteIntentError.Get(&cArgs.EvalCtx.ClusterSettings().SV); m > 0 {
 		maxIntents = uint64(m)
@@ -191,7 +173,6 @@ func evalExport(
 			MaxSize:            maxSize,
 			MaxIntents:         maxIntents,
 			StopMidKey:         args.SplitMidKey,
-			ResourceLimiter:    storage.NewResourceLimiter(storage.ResourceLimiterOptions{MaxRunTime: maxRunTime}, timeutil.DefaultTimeSource{}),
 		}
 		var summary roachpb.BulkOpSummary
 		var resume storage.MVCCKey

diff --git a/pkg/storage/BUILD.bazel b/pkg/storage/BUILD.bazel
@@ -33,7 +33,6 @@ go_library(
         "point_synthesizing_iter.go",
         "read_as_of_iterator.go",
         "replicas_storage.go",
-        "resource_limiter.go",
         "row_counter.go",
         "slice.go",
         "slice_go1.9.go",
@@ -42,7 +41,6 @@ go_library(
         "store_properties.go",
         "temp_engine.go",
         "verifying_iterator.go",
-        ":gen-resourcelimitreached-stringer",  # keep
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/storage",
     visibility = ["//visibility:public"],
@@ -128,7 +126,6 @@ go_test(
         "pebble_mvcc_scanner_test.go",
         "pebble_test.go",
         "read_as_of_iterator_test.go",
-        "resource_limiter_test.go",
         "sst_test.go",
         "sst_writer_test.go",
         "temp_engine_test.go",
@@ -187,10 +184,4 @@ go_test(
     ],
 )
 
-stringer(
-    name = "gen-resourcelimitreached-stringer",
-    src = "resource_limiter.go",
-    typ = "ResourceLimitReached",
-)
-
 get_x_data(name = "get_x_data")
diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go
@@ -6133,8 +6133,6 @@ func mvccExportToWriter(
 
 	paginated := opts.TargetSize > 0
 	hasElasticCPULimiter := elasticCPUHandle != nil
-	hasTimeBasedResourceLimiter := opts.ResourceLimiter != nil
-
 	// trackKeyBoundary is true if we need to know whether the
 	// iteration has proceeded to a new key.
 	//
@@ -6145,8 +6143,7 @@ func mvccExportToWriter(
 	// key boundaries if we may return from our iteration before
 	// the EndKey. This can happen if the user has requested
 	// paginated results, or if we hit a resource limit.
-	trackKeyBoundary := opts.ExportAllRevisions && (paginated || hasTimeBasedResourceLimiter || hasElasticCPULimiter)
-
+	trackKeyBoundary := opts.ExportAllRevisions && (paginated || hasElasticCPULimiter)
 	firstIteration := true
 	// skipTombstones controls whether we include tombstones.
 	//
@@ -6221,30 +6218,6 @@ func mvccExportToWriter(
 			// of starvation. Otherwise operations could spin indefinitely.
 			firstIteration = false
 		} else {
-			// TODO(irfansharif): Remove this time-based resource limiter once
-			// enabling elastic CPU limiting by default. There needs to be a
-			// compelling reason to need two mechanisms.
-			if opts.ResourceLimiter != nil {
-				// In happy day case we want to only stop at key boundaries as it allows callers to use
-				// produced sst's directly. But if we can't find key boundary within reasonable number of
-				// iterations we would split mid key.
-				// To achieve that we use soft and hard thresholds in limiter. Once soft limit is reached
-				// we would start searching for key boundary and return as soon as it is reached. If we
-				// can't find it before hard limit is reached and caller requested mid key stop we would
-				// immediately return.
-				limit := opts.ResourceLimiter.IsExhausted()
-				// We can stop at key once any threshold is reached or force stop at hard limit if midkey
-				// split is allowed.
-				if limit >= ResourceLimitReachedSoft && isNewKey || limit == ResourceLimitReachedHard && opts.StopMidKey {
-					// Reached iteration limit, stop with resume span
-					resumeKey = unsafeKey.Clone()
-					if isNewKey {
-						resumeKey.Timestamp = hlc.Timestamp{}
-					}
-					break
-				}
-			}
-
 			// Check if we're over our allotted CPU time + on a key boundary (we
 			// prefer callers being able to use SSTs directly). Going over limit is
 			// accounted for in admission control by penalizing the subsequent
@@ -6505,10 +6478,6 @@ type MVCCExportOptions struct {
 	// cause problems with multiplexed iteration using NewSSTIterator(), nor when
 	// ingesting the SSTs via `AddSSTable`.
 	StopMidKey bool
-	// ResourceLimiter limits how long iterator could run until it exhausts allocated
-	// resources. Export queries limiter in its iteration loop to break out once
-	// resources are exhausted.
-	ResourceLimiter ResourceLimiter
 	// FingerprintOptions controls how fingerprints are generated
 	// when using MVCCExportFingerprint.
 	FingerprintOptions MVCCExportFingerprintOptions