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.

Sign up for GitHub

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

Jump to bottom

sql: make the type schema changer cleanup resilient to retryable errors #60489

Closed
arulajmani opened this issue Feb 11, 2021 · 0 comments · Fixed by #60495
Closed

sql: make the type schema changer cleanup resilient to retryable errors #60489

arulajmani opened this issue Feb 11, 2021 · 0 comments · Fixed by #60495
Assignees
@arulajmani
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@arulajmani
Copy link
Collaborator

Currently, the type schema changer's OnFailrOrCancel method doesn't disambiguate retryable errors and mark them as retryable. As a result, it could leave descriptors in a state where manual cleanup could be required even though the operation should've been retried. We should fix this, by doing something similar to what the regular schema changer does:

cockroach/pkg/sql/schema_changer.go

Line 2439 in 81e617e

case !isPermanentSchemaChangeError(rollbackErr):

It's worth noting that Resume accounts for retryable errors correctly.

cc @lucy-zhang from our discussion the other day
@arulajmani arulajmani added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Feb 11, 2021
@arulajmani arulajmani self-assigned this Feb 11, 2021
arulajmani added a commit to arulajmani/cockroach that referenced this issue Feb 11, 2021
@arulajmani
sql: ensure type schema change cleanup job is resilient to retries
b6b6170 
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 mentioned this issue Feb 11, 2021
Merged
craig bot pushed a commit that referenced this issue Feb 16, 2021
@arulajmani @adityamaru
Merge #60495 #60543
0c62e88 
60495: sql: ensure type schema change cleanup job is resilient to retries r=lucy-zhang,otan a=arulajmani

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.

60543: builtins: change complete_stream builtin to take a timestamp r=pbardea,miretskiy a=adityamaru

This change adds a ts parameter to
crdb_internal.complete_stream_ingestion_job builtin. This ts will be the
ts as of which the cluster being ingested into will be considered in a
consistent state.

Release note: None

Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
@craig craig bot closed this as completed in 755fa25 Feb 16, 2021
arulajmani added a commit to arulajmani/cockroach that referenced this issue Feb 19, 2021
@arulajmani
sql: ensure type schema change cleanup job is resilient to retries
5b621e2 
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 mentioned this issue Feb 19, 2021
Merged
arulajmani added a commit to arulajmani/cockroach that referenced this issue Feb 22, 2021
@arulajmani
sql: ensure type schema change cleanup job is resilient to retries
f642792 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arulajmani arulajmani

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: ensure type schema change cleanup job is resilient to retries arulajmani/cockroach
1 participant
@arulajmani