sql: add RevertSpans and RevertSpansFanout #97845

stevendanna · 2023-03-01T14:24:28Z

This PR adds RevertSpans and RevertSpansFanout.

RevertSpans de-duplicates some code between import rollback and streaming cutover.

Along the way, it fixes a small bug that existed in RevertTables: Only a single RevertRangeRequest is permitted in a batch since RevertRangeRequest has the isAlone flag set. As a result, a future caller of RevertTables would have encountered a fatal error from KV.

RevertSpansFanout uses DistSQL's PartitionSpans to manually fan out multiple RevertRange requests. Since all users of revert range request currently set a limit on the number of keys touched, dist sender doesn't fanout such request.

Epic: none

Release note: None

cockroach-teamcity · 2023-03-01T14:24:36Z

This change is

yuzefovich

Reviewed 3 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler, @rhu713, @rytaft, and @stevendanna)

pkg/sql/revert.go line 49 at r1 (raw file):

var maxRevertSpanNumWorkers = settings.RegisterIntSetting(
	settings.TenantWritable,
	"max_revert_span_parallelism",

nit: we usually have several parts in a cluster setting name, delimited by periods, with the first part being about the layer, so perhaps name it something like sql.revert.max_span_parallelism.

pkg/sql/revert.go line 51 at r1 (raw file):

	"max_revert_span_parallelism",
	"the maximum number of workers used to issue RevertRange request",
	8)

Do we want to add NonNegativeInt or PositiveInt validation?

pkg/sql/revert.go line 140 at r1 (raw file):

		workerPartitions = spanPartitions
	} else {
		workerPartitions = make([]SpanPartition, maxWorkerCount)

If we allow the setting to be set to 0, then we need to handle zero case differently.

pkg/sql/revert.go line 150 at r1 (raw file):

	progressWorker := func(ctx context.Context) error {
		for sp := range completedSpanCh {
			if onCompletedCallback != nil {

nit: perhaps take the if out of the loop and perhaps just short-circuit if the calback is nil.

stevendanna

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler, @rhu713, and @yuzefovich)

pkg/sql/revert.go line 51 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Do we want to add NonNegativeInt or PositiveInt validation?

Thanks! PositiveInt should do.

pkg/sql/revert.go line 140 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

If we allow the setting to be set to 0, then we need to handle zero case differently.

Added the PositiveInt validation so the zero case should be OK now.

pkg/sql/revert.go line 150 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps take the if out of the loop and perhaps just short-circuit if the calback is nil.

While addressing this comment, I noticed that I wasn't really handling the error return from this callback correctly. So, perhaps I've gone a bit overboard, but I've done a few things:

short-circuit the 1 worker case to avoid all planning and just call RevertSpans
short-circuit the case of a nil onCompletedCallback to avoid the extra layer of goroutines altogether
Added an errCh that workers get the last onCompletedCallback error back from.

msbutler

this is awesome! eager to see benchmarks!

pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

msbutler · 2023-03-02T21:17:42Z

pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

+// the cutover process.
+type cutoverProgressTracker struct {
+	minProgressUpdateInterval time.Duration
+	progMetric                *metric.Gauge


nit: rename this rangesToDoMetric or something a bit more descriptive? It seems hardcoded to update the number of remaining ranges.

pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

msbutler · 2023-03-03T15:03:15Z

pkg/sql/revert.go

+			span = resp.ResumeSpan
+
+			if resp.ResumeSpan != nil {
+				if !resp.ResumeSpan.Valid() {


nit: perhaps we add some logging around resume key spans and the number of attempts to revert the original span?

We have a VEventf above that can be added on each revert range call. Note that a resume span is absolutely expected.

pkg/sql/revert_test.go

pkg/sql/revert.go

msbutler · 2023-03-03T15:37:58Z

pkg/sql/revert.go

+	settings.TenantWritable,
+	"sql.revert.max_span_parallelism",
+	"the maximum number of workers used to issue RevertRange request",
+	8,


should this scale by the number of nodes in the cluster?

Under the race detector, kvclient uses a transport that disallows mutations of of a request send to the transport. This change ensures we create a new request for each resume span rather than re-using the old request. Epic: none Release note: None

This PR adds RevertSpans and RevertSpansFanout. RevertSpans de-duplicates some code between import rollback and streaming cutover. Along the way, it fixes a small bug that existed in RevertTables: Only a single RevertRangeRequest is permitted in a batch since RevertRangeRequest has the isAlone flag set. As a result, a future caller of RevertTables would have encountered a fatal error from KV. RevertSpansFanout uses DistSQL's PartitionSpans to manually fan out multiple RevertRange requests. Since all users of revert range request currently set a limit on the number of keys touched, dist sender doesn't fanout such request. Release note: None

Release note: None

Epic: none Release note: None

msbutler · 2023-03-18T21:45:03Z

pkg/sql/sem/builtins/builtins.go

@@ -7568,12 +7568,6 @@ expires until the statement bundle is collected`,
 					filter = kvpb.MVCCFilter_All
 				}

-				req := &kvpb.ExportRequest{


this commit merged elsewhere, correct?

Oh did it? Let me go check, perhaps I hadn't git-pulled before working on this.

pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

pkg/sql/revert.go

pkg/sql/revert_test.go

stevendanna · 2023-03-19T08:37:12Z

bors r=msbutler

craig · 2023-03-19T09:20:24Z

Build failed:

Bazel Essential CI (Cockroach)

stevendanna · 2023-03-19T15:19:00Z

bors retry

craig · 2023-03-19T16:03:05Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-19T18:17:22Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-19T19:36:57Z

Build succeeded:

Bazel Essential CI (Cockroach)

stevendanna requested review from a team as code owners March 1, 2023 14:24

stevendanna requested review from rhu713 and rytaft and removed request for a team March 1, 2023 14:24

stevendanna requested a review from msbutler March 1, 2023 14:26

stevendanna force-pushed the parallel-revert-range branch from 6af38f9 to 40db612 Compare March 1, 2023 14:57

yuzefovich reviewed Mar 1, 2023

View reviewed changes

rytaft removed their request for review March 1, 2023 22:12

stevendanna force-pushed the parallel-revert-range branch from 40db612 to ff6c3a7 Compare March 2, 2023 11:57

stevendanna commented Mar 2, 2023

View reviewed changes

msbutler reviewed Mar 3, 2023

View reviewed changes

stevendanna force-pushed the parallel-revert-range branch from ff6c3a7 to fbc3106 Compare March 3, 2023 17:54

stevendanna force-pushed the parallel-revert-range branch from fbc3106 to 2ade877 Compare March 17, 2023 22:46

stevendanna added 4 commits March 18, 2023 17:35

sql: use a mutex rather than channels in RevertSpansFanout

67ba17f

Release note: None

roachtest: use capability to opt tenant out of rate limiting

5adc4f2

Epic: none Release note: None

stevendanna force-pushed the parallel-revert-range branch from 2ade877 to 5adc4f2 Compare March 18, 2023 17:38

stevendanna requested a review from a team as a code owner March 18, 2023 17:38

stevendanna requested review from a team, herkolategan and srosenberg and removed request for a team March 18, 2023 17:38

msbutler mentioned this pull request Mar 18, 2023

import: checkpoint rollback progress #97385

Open

msbutler approved these changes Mar 18, 2023

View reviewed changes

craig bot merged commit 9aa4f34 into cockroachdb:master Mar 19, 2023

msbutler added a commit to msbutler/cockroach that referenced this pull request Mar 24, 2023

update crdb_internal fingerprint per cockroachdb#97845

dd99d38

msbutler mentioned this pull request Mar 31, 2023

ccl/streamingccl/streamingest: TestRandomClientGeneration failed #99343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add RevertSpans and RevertSpansFanout #97845

sql: add RevertSpans and RevertSpansFanout #97845

stevendanna commented Mar 1, 2023 •

edited

Loading

cockroach-teamcity commented Mar 1, 2023

yuzefovich left a comment

stevendanna left a comment

msbutler left a comment

msbutler Mar 2, 2023

msbutler Mar 3, 2023

stevendanna Mar 18, 2023

msbutler Mar 3, 2023

msbutler Mar 18, 2023

stevendanna Mar 19, 2023

stevendanna commented Mar 19, 2023

craig bot commented Mar 19, 2023

stevendanna commented Mar 19, 2023

craig bot commented Mar 19, 2023

craig bot commented Mar 19, 2023

craig bot commented Mar 19, 2023

sql: add RevertSpans and RevertSpansFanout #97845

sql: add RevertSpans and RevertSpansFanout #97845

Conversation

stevendanna commented Mar 1, 2023 • edited Loading

cockroach-teamcity commented Mar 1, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

msbutler Mar 2, 2023

Choose a reason for hiding this comment

msbutler Mar 3, 2023

Choose a reason for hiding this comment

stevendanna Mar 18, 2023

Choose a reason for hiding this comment

msbutler Mar 3, 2023

Choose a reason for hiding this comment

msbutler Mar 18, 2023

Choose a reason for hiding this comment

stevendanna Mar 19, 2023

Choose a reason for hiding this comment

stevendanna commented Mar 19, 2023

craig bot commented Mar 19, 2023

stevendanna commented Mar 19, 2023

craig bot commented Mar 19, 2023

craig bot commented Mar 19, 2023

craig bot commented Mar 19, 2023

stevendanna commented Mar 1, 2023 •

edited

Loading