backupccl: add multi-region test to TestRestoreOldVersions#69725
backupccl: add multi-region test to TestRestoreOldVersions#69725craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
ajstorm
left a comment
There was a problem hiding this comment.
Reviewed 1 of 18 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan and @shermanCRL)
pkg/ccl/backupccl/restore_old_versions_test.go, line 294 at r1 (raw file):
} func runOldVersionMultiRegionTest(exportDir string) func(t *testing.T) {
This LGTM, but I'm wondering if we need to test a few more permutations. For example, what about table level restore? We currently don't support it for MR tables into MR databases (which we should validate), but we do support non-MR tables into MR databases.
pkg/ccl/backupccl/restore_old_versions_test.go, line 326 at r1 (raw file):
}) defer tc.Stopper().Stop(ctx) require.NoError(t, os.Symlink(exportDir, filepath.Join(dir, "foo")))
Nit: why "foo"? Does it make sense to give this a more descriptive name? Something like "localBackupDir"?
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/backupccl/restore_old_versions_test.go, line 294 at r1 (raw file):
Previously, ajstorm (Adam Storm) wrote…
This LGTM, but I'm wondering if we need to test a few more permutations. For example, what about table level restore? We currently don't support it for MR tables into MR databases (which we should validate), but we do support non-MR tables into MR databases.
eh there's other tests that already do this no?
pkg/ccl/backupccl/restore_old_versions_test.go, line 326 at r1 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: why "foo"? Does it make sense to give this a more descriptive name? Something like "localBackupDir"?
Copy paste. All the other tests do this.
ajstorm
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/backupccl/restore_old_versions_test.go, line 294 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
eh there's other tests that already do this no?
Yes, but not for older versions. When we relax the restrictions, I'd think we'd want to ensure that they work on older versions. Not suggesting we do it here (although maybe we should), but another issue/PR could be warranted.
pkg/ccl/backupccl/restore_old_versions_test.go, line 326 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Copy paste. All the other tests do this.
Why be like everyone else?
|
pkg/ccl/backupccl/restore_old_versions_test.go, line 294 at r1 (raw file): Previously, ajstorm (Adam Storm) wrote…
Yeah I'd think there's a million cases to test in theory, I don't buy its too much more exciting. In general I'd rather a randomised backup restore test. |
|
pkg/ccl/backupccl/restore_old_versions_test.go, line 326 at r1 (raw file): Previously, ajstorm (Adam Storm) wrote…
Just want to fit in |
Release justification: test-only change Release note: None
ajstorm
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/backupccl/restore_old_versions_test.go, line 294 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Yeah I'd think there's a million cases to test in theory, I don't buy its too much more exciting. In general I'd rather a randomised backup restore test.
The fact that the enum isn't coming along for the ride makes things more interesting IMO. Doesn't have to be this PR, but I do think that we should open an issue.
ajstorm
left a comment
There was a problem hiding this comment.
Reviewable status:
|
bors r=ajstorm |
|
Build succeeded: |
This change removes an old MR backup that was being restored and tested against. There were no instructions on how to regenerate the fixture in cockroachdb#69725 so it is being deleted. Release note: None
This change removes an old MR backup that was being restored and tested against. There were no instructions on how to regenerate the fixture in cockroachdb#69725 so it is being deleted. Release note: None
Release justification: test-only change
Release note: None
Resolves #67650