-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql,backupccl: create IMPORT ROLLBACK job in online restore #120407
sql,backupccl: create IMPORT ROLLBACK job in online restore #120407
Conversation
4cf9126
to
1615358
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I gave a pass at the non-test code. It looks like your test failed in CI, so I'll wait to look at it once it's green.
} | ||
|
||
func (r *importRollbackResumer) OnFailOrCancel(context.Context, interface{}, error) error { | ||
return errors.AssertionFailedf("OnFailOrCancel called on non-cancellable job %d", r.job.ID()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i may have missed this somewhere: do you have to flag in job creation or in the job constructor that this job is non-cancellable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, added the bool to the job record.
23d69f7
to
1a032c9
Compare
Yeah, I semantically merged-conflicted with myself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! just had one question about the unit test
exec-sql | ||
BACKUP INTO 'nodelocal://1/cluster/'; | ||
---- | ||
|
||
|
||
new-cluster name=s2 share-io-dir=s1 allow-implicit-access disable-tenant | ||
new-cluster name=s2 allow-implicit-access disable-tenant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need to use a new cluster. this test was written before prefix synthesis.
pkg/ccl/backupccl/testdata/backup-restore/online-restore-in-progress-imports
Outdated
Show resolved
Hide resolved
d955392
to
1ed770f
Compare
bors r=msbutler |
Build failed: |
Tables that are offline and have a non-zero ImportEpoch are now restored to their offline state and an IMPORT ROLLBACK job is synthesized. The IMPORT ROLLBACK job uses an epoch-based predicate delete to rollback the table and bring it back online. Epic: none Release note: None
1ed770f
to
f2790a1
Compare
bors r=msbutler |
Tables that are offline and have a non-zero ImportEpoch are now restored to their offline state and an IMPORT
ROLLBACK job is synthesized.
The IMPORT ROLLBACK job uses an epoch-based predicate delete to
rollback the table and bring it back online.
Epic: none
Release note: None