Skip to content
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: ensure type schema change cleanup job is resilient to retries #60495

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

arulajmani
Copy link
Collaborator

Previously if the type schema changer ran into a non-permanent error,
it wouldn't retry transparently. Instead, manual cleanup would be
required. This patch fixes this behavior.

This patch also adds a testing knob, RunAfterOnFailOrCancel to test
the afformentioned bug.

Fixes #60489

Release note (bug fix): Previosly, retryable errors in the cleanup
phase of the type schema changer wouldn't be retried automatically
in the background. This is now fixed.

@arulajmani arulajmani requested review from thoszhang, otan and a team February 11, 2021 18:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@arulajmani
Copy link
Collaborator Author

@otan I think the testing knob belongs in this commit, as I think we may wanna backport this thing to 20.2.

"descriptor %d not found for type change job; assuming it was dropped, and exiting",
tc.typeID,
)
case !isPermanentSchemaChangeError(rollbackErr):
Copy link
Contributor

@otan otan Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure i trust this function enough to not leave dangling references but i have not enough experience with it ;)

LGTM is LGT-schema!

Previously if the type schema changer ran into a non-permanent error,
it wouldn't retry transparently. Instead, manual cleanup would be
required. This patch fixes this behavior.

This patch also adds a testing knob, `RunAfterOnFailOrCancel` to test
the afformentioned bug.

Fixes cockroachdb#60489

Release note (bug fix): Previosly, retryable errors in the cleanup
phase of the type schema changer wouldn't be retried automatically
in the background. This is now fixed.
@arulajmani arulajmani force-pushed the type-changer-retryable branch from b6b6170 to 755fa25 Compare February 11, 2021 21:53
@blathers-crl blathers-crl bot requested a review from otan February 11, 2021 21:53
@thoszhang thoszhang requested a review from a team February 12, 2021 00:40
Copy link
Contributor

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

@arulajmani
Copy link
Collaborator Author

bors r=lucy-zhang,otan

@craig
Copy link
Contributor

craig bot commented Feb 16, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: make the type schema changer cleanup resilient to retryable errors
4 participants