backupbase,backupdest,backuputils: introduce new backup packages by adityamaru · Pull Request #82381 · cockroachdb/cockroach

adityamaru · 2022-06-02T22:32:22Z

This change introduces three new packages:

backupdest - all logic related to backup destination resolution
backuputils - shared utility and test methods
backupbase - shared constants and cluster settings

In order to minimize the churn, this change focuses on moving methods
and constants needed to carve out backupdestination. There are TODOs
that will be addressed in follow up PRs to centralize shared methods and
variables such as cluster settings, test setup helpers etc. There will
also be a fast follow introduction of a backupinfo package that will contain
all the logic related to backup manifest construction and resolution.

This is a refactor only change.

Release note: None

cockroach-teamcity · 2022-06-02T22:32:29Z

This change is

benbardin · 2022-06-03T15:25:53Z

pkg/ccl/backupccl/backupbase/constants.go

@@ -0,0 +1,51 @@
+// Copyright 2022 The Cockroach Authors.


Since backupccl is in the path, the prefix backup in backupbase seems redundant? Maybe just name the packages base, destination, and utils?

hmmm so when these packages are used by others it'll be backupbase.foo which might be more informative than just base.foo? I agree the stutter is not ideal but I think we have some precedence here:

changfeedccl/changefeedbase, kv/kvserver/kvbase being a few of them.

The schemachanger package settled on a sort abbreviation for their "sub packages" (scexec, scplan), but I'm not sure bk or bu have the same ring to them.

I was OOO so no need to revisit, but I'm comfortable putting it on importers to use import aliasing!

dt · 2022-06-03T15:39:47Z

pkg/ccl/backupccl/backupbase/settings.go

+	"github.com/cockroachdb/cockroach/pkg/util"
+)
+
+// TODO(adityamaru): Move all backup related cluster settings to this file.


Is this a goal?

If a setting is only used during destination resolution, or only used during data exporting, etc, moving it here is the opposite of dependency hygiene; now if we change this setting, or add another one, etc, we're needlessly adding churn to a dependency of other packages, because something specific to one package leaked out into a common package.

makes sense, I needed this setting in a place that I could import into testutils.go so that it is registered at the time the test cluster is started. I'll remove the TODO but I don't think I'd want testutils.go to import any of backupdestination or backupinfo so I think I'll need to keep this file around?

stevendanna

Overall, huge kudos for starting this process.

This will increase the cost of backports for us for this release, but I think we should probably just pay that cost now.

I've left a few comments, but I assume this is something we are going to iterate on over time, so none of them are really blocking.

stevendanna · 2022-06-06T09:12:03Z

pkg/ccl/backupccl/backupbase/constants.go

@@ -0,0 +1,51 @@
+// Copyright 2022 The Cockroach Authors.


The schemachanger package settled on a sort abbreviation for their "sub packages" (scexec, scplan), but I'm not sure bk or bu have the same ring to them.

stevendanna · 2022-06-06T09:13:08Z

pkg/ccl/backupccl/backupbase/constants.go

+
+package backupbase
+
+const (


I'm OK with this since we are trying to make incremental progress here. But I think some of these constants eventually belong in a more appropriate package. For example, many of them have to do with the layout of the backup collection itself.

Yup I agree, some of these constants are used in test files, backupdest, debug_backup as well as manifest handling. I want to introduce backupinfo to better understand the dependency graph before we move these into specific packages.

pkg/ccl/backupccl/backupdestination/backup_destination.go

stevendanna · 2022-06-06T09:18:11Z

pkg/ccl/backupccl/backupbase/constants.go

+	IncludeManifest = true
+
+	// OmitManifest is a named const that can be passed to FindPriorBackups.
+	OmitManifest = false


Since these two are only used as arguments to a function that lives in backupdestination perhaps we can move them there now.

Currently IncludeManifest is used in manifest_handling.go which will move into backupinfo. OmitManifest is used backupdest as well as cliccl.

Now that I'm looking at the method resolveBackupManifests that uses IncludeManifest in manifest_handling.go, this will cause issues when moving manifest_handling.go into backupinfo since backupinfo should probably not depend on backupdest?

Perhaps I misread, but the only uses of these are in calls to FindPriorBackups. Since FindPriorBackups is in backupdest, then anything that depends on these constants already depends on backupdest. So, the only way to break the dependency is to move FindPriorBackups and then these constants can live in whatever package that function lives in.

All that said, you have this dependency graph more in mind than I do at the moment, and I think we can do this iteratively so don't treat this as blocking.

Chatted offline, this is a dependency we are going to have to break at some point. Whether we do this through dependency injection/interfaces is still to be seen. I think we agree that we should draw the lines in the sand with these packages and then iterate on cleaning up the dependency graph. I might capture some of these improvements in a TODO list issue just so that we don't forget.

pkg/ccl/backupccl/backupdestination/backup_destination_test.go

stevendanna · 2022-06-06T09:24:41Z

pkg/ccl/backupccl/backuputils/utils.go

+// to accidentally correct an unauthorized file path to an authorized one, then
+// write a backup to an unexpected place or print the wrong error message on
+// a restore.
+func JoinURLPath(args ...string) string {


I could see this living in the cloud package since it is particular to our external storage implementation. But, we can leave that bikeshed to a follow-up PR if anyone else agrees.

This change introduces three new packages: `backupdest` - all logic related to backup destination resolution `backuputils` - shared utility and test methods `backupbase` - shared constants and cluster settings In order to minimize the churn, this change focuses on moving methods and constants needed to carve out `backupdest`. There are TODOs that will be addressed in follow up PRs to centralize shared methods and variables such as cluster settings, test setup helpers etc. There will also be a fast follow introduction of a `backupinfo` package that will contain all the logic related to backup manifest construction and resolution. This is a refactor only change. Release note: None

adityamaru · 2022-06-07T18:26:12Z

Checking this in, but feel free to comment or open up changes to improve this!

TFTR!

bors r+

craig · 2022-06-07T19:28:34Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-07T22:51:13Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru mentioned this pull request Jun 2, 2022

backupccl: remove destination resolution during planning #82366

Merged

adityamaru force-pushed the introduce-backupdestination branch 2 times, most recently from b398f9b to 83fdf90 Compare June 3, 2022 14:30

adityamaru changed the title ~~backupdestination,backuputils: introduce new backup packages~~ backupbase,backupdestination,backuputils: introduce new backup packages Jun 3, 2022

adityamaru marked this pull request as ready for review June 3, 2022 14:34

adityamaru requested a review from a team as a code owner June 3, 2022 14:34

adityamaru requested review from a team, benbardin, dt, msbutler and stevendanna and removed request for a team and stevendanna June 3, 2022 14:34

benbardin reviewed Jun 3, 2022

View reviewed changes

dt reviewed Jun 3, 2022

View reviewed changes

adityamaru requested review from benbardin and dt June 3, 2022 17:20

stevendanna reviewed Jun 6, 2022

View reviewed changes

adityamaru requested a review from stevendanna June 7, 2022 13:58

adityamaru force-pushed the introduce-backupdestination branch 2 times, most recently from 99e67b5 to 2180634 Compare June 7, 2022 14:27

adityamaru changed the title ~~backupbase,backupdestination,backuputils: introduce new backup packages~~ backupbase,backupdest,backuputils: introduce new backup packages Jun 7, 2022

adityamaru force-pushed the introduce-backupdestination branch from 2180634 to 5c91ef6 Compare June 7, 2022 16:05

craig bot merged commit 7d7f2d4 into cockroachdb:master Jun 7, 2022

adityamaru deleted the introduce-backupdestination branch June 7, 2022 23:57

adityamaru mentioned this pull request Jun 24, 2022

backupccl: refactor backupccl to introduce smaller packages #83338

Closed

Conversation

adityamaru commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 2, 2022

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!

adityamaru Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevendanna 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jun 7, 2022

Uh oh!

craig bot commented Jun 7, 2022

Uh oh!

craig bot commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adityamaru commented Jun 2, 2022 •

edited

Loading

adityamaru Jun 3, 2022 •

edited

Loading