backupccl: move descriptors and descriptor changes field from manifest to SST by rhu713 · Pull Request #96245 · cockroachdb/cockroach

rhu713 · 2023-01-30T22:46:30Z

As part of an effort to make backup manifests scale better for larger clusters, this patch moves descriptors and descriptor changes from the manifest to an external SST. This avoids the need to alloc enough memory to hold every descriptor and descriptor revision for every layer of a backup during a backup or restore job. This patch also changes the access pattern for descriptors and descriptor changes to use iterators, so that they can be accessed in a streaming manner from the external SST.

Release note: None

cockroach-teamcity · 2023-01-30T22:46:38Z

This change is

adityamaru · 2023-02-01T14:24:35Z

I'm going to review this today, adding @dt as well in case he wants to take a look.

pkg/ccl/backupccl/show.go

adityamaru

Flushing out my first round of comments, overall it looks really good!

adityamaru · 2023-02-02T15:29:30Z

pkg/ccl/backupccl/backupinfo/manifest_handling.go

 // a slice of mutable descriptors.
 func BackupManifestDescriptors(
-	backupManifest *backuppb.BackupManifest,
+	ctx context.Context, backupManifest *backuppb.BackupManifest, iterFactory *IterFactory,


nit: I'd prefer if we stopped passing in backupManifest to this method, and only passed in the EndTime. I like the mental model of only using the iter to access descriptors from a manifest, and it was a little confusing seeing us also pass in the manifest to this method.

done, changed to pass in the end time.

pkg/ccl/backupccl/backupinfo/manifest_handling.go

adityamaru · 2023-02-02T15:42:59Z

pkg/ccl/backupccl/backupinfo/manifest_handling.go

+func (f *IterFactory) NewFileIter(
+	ctx context.Context,
+) (bulk.Iterator[*backuppb.BackupManifest_File], error) {
+	if f.m.HasExternalFilesList {


nit: can we rename the proto field now that it encompasses more than just Files. Maybe HasExternalSSTs?

done, changed to HasExternalManifestSSTs

adityamaru · 2023-02-02T15:52:40Z

pkg/ccl/backupccl/backupinfo/backup_metadata.go

+		return err
+	}
+	defer w.Close()
+	fileSST := storage.MakeBackupSSTWriter(ctx, dest.Settings(), w)


nit: rename to descsSST

adityamaru · 2023-02-02T15:55:13Z

pkg/ccl/backupccl/backup_metadata_test.go

 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
-	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/bulk"


Is there any reason this file isn't in backupinfo? Doesn't have to be done in this PR but just curious if you knew.

I thought it was more of a util method not just related to backups so I put it in the utils package.

ahh sorry I meant why is backup_metadata_test.go in backupccl as compared to backupinfo? Anyways, we can move it in a smaller followup if need be!

adityamaru · 2023-02-02T15:56:55Z

pkg/ccl/backupccl/full_cluster_backup_restore_test.go

 			t.Fatal(err)
 		}
-		if info.Name() == backupbase.BackupManifestName || !strings.HasSuffix(path, ".sst") {
+		if info.Name() == backupbase.BackupManifestName || !strings.HasSuffix(path, ".sst") || info.Name() == backupinfo.BackupMetadataDescriptorsListPath {


do we need to add files list sst to this check as well?

yeah I think it's better to add the files list SST to this exclusion list just so we're not relying on existing behavior for this test to pass.

adityamaru · 2023-02-02T17:08:36Z

pkg/ccl/backupccl/restore_planning.go

 		return nil
 	}

+	// We do not need to upgrade descriptors that exist in the external SST


I like this, and I'll probably delete this soon once we restrict restores to our new policy. In the meantime though can we check backupManifests[0].ClusterVersion instead?

done, used cluster version.

pkg/ccl/backupccl/restore_span_covering.go

adityamaru · 2023-02-13T13:36:03Z

pkg/ccl/backupccl/backup_metadata_test.go

 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
-	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/bulk"


ahh sorry I meant why is backup_metadata_test.go in backupccl as compared to backupinfo? Anyways, we can move it in a smaller followup if need be!

adityamaru · 2023-02-13T13:46:27Z

pkg/ccl/backupccl/backupinfo/backup_metadata.go

@@ -1269,100 +1367,116 @@ func (dri *DescriptorRevisionIterator) Err() error {
 // revision, and false if there are no more elements or if an error was


nit: the comment specifies we return true/false but I don't think thats the case anymore.

adityamaru · 2023-02-13T13:48:34Z

pkg/ccl/backupccl/backupinfo/backup_metadata.go

 			// descriptors at the start time, so we don't have to look back further
 			// than the very last backup.
-			if m.StartTime.IsEmpty() {
+			if m.StartTime.IsEmpty() || m.MVCCFilter == backuppb.MVCCFilter_Latest {


was this previously a bug? if so do we have a test for it now.

pkg/ccl/backupccl/backupinfo/manifest_handling.go

adityamaru · 2023-02-13T14:42:57Z

pkg/ccl/backupccl/backupinfo/manifest_handling.go

+			continue
+		}
+
+		if rev.ID == prevRevID {


maybe it's just me but the sorting comment is a little far from this logic. Can I bug you to add a sentence that since revisions are sorted by ID and the descending order of revisions we can skip over older revisions once we have seen a revision for an ID.

adityamaru · 2023-02-13T14:48:50Z

I'd really like if we added a mixed-version database/table restore roachtest. I'll file an issue for this that we should try to get to before backporting this.

…t to SST As part of an effort to make backup manifests scale better for larger clusters, this patch moves descriptors and descriptor changes from the manifest to an external SST. This avoids the need to alloc enough memory to hold every descriptor and descriptor revision for every layer of a backup during a backup or restore job. This patch also changes the access pattern for descriptors and descriptor changes to use iterators, so that they can be accessed in a streaming manner from the external SST. Release note: None

rhu713 · 2023-02-14T19:40:16Z

bors r+

craig · 2023-02-14T20:57:22Z

Build succeeded:

Bazel Essential CI (Cockroach)

rhu713 force-pushed the descriptor-iter branch 3 times, most recently from e6f4520 to 90efbb5 Compare January 31, 2023 12:52

rhu713 requested a review from adityamaru January 31, 2023 12:52

rhu713 marked this pull request as ready for review January 31, 2023 12:53

rhu713 requested review from a team as code owners January 31, 2023 12:53

adityamaru requested a review from dt February 1, 2023 14:24

adityamaru reviewed Feb 2, 2023

View reviewed changes

pkg/ccl/backupccl/show.go Show resolved Hide resolved

adityamaru suggested changes Feb 2, 2023

View reviewed changes

adityamaru self-requested a review February 2, 2023 17:20

rhu713 force-pushed the descriptor-iter branch 9 times, most recently from 06c6435 to 13eb67a Compare February 8, 2023 17:38

adityamaru approved these changes Feb 13, 2023

View reviewed changes

rhu713 force-pushed the descriptor-iter branch 4 times, most recently from 742a2c6 to 0d9bd2a Compare February 14, 2023 17:10

rhu713 force-pushed the descriptor-iter branch from 0d9bd2a to 002f294 Compare February 14, 2023 17:20

craig bot merged commit 186fa03 into cockroachdb:master Feb 14, 2023

rhu713 mentioned this pull request Feb 15, 2023

release-22.2: move alloc heavy Files field from manifest to SST, use slim manifest in backup restore #97210

Merged

		@@ -1269,100 +1367,116 @@ func (dri *DescriptorRevisionIterator) Err() error {
		// revision, and false if there are no more elements or if an error was

Conversation

rhu713 commented Jan 30, 2023

Uh oh!

cockroach-teamcity commented Jan 30, 2023

Uh oh!

adityamaru commented Feb 1, 2023

Uh oh!

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Feb 13, 2023

Uh oh!

rhu713 commented Feb 14, 2023

Uh oh!

craig bot commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants