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/gcjob: retry failed GC jobs #65910

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Conversation

sajjadrizvi
Copy link

In the previous implementation, failed GC jobs were not being retried regardless
whether the failure is permanent or transient. As a result, a GC job's failure
risked orphaned data, which cannot be reclaimed.

This commit adds a mechanism to retry failed GC jobs that are not permanent. No
limit is set on the number of retries. For the time being, the failure type is
determined based on the failure categorization of schema-change jobs. This
behavior is expected to change once exponential backoff mechanism is
implemented for failed jobs (#44594).

Release note: None

Fixes: #65000

Release note (<category, see below>):

@sajjadrizvi sajjadrizvi requested review from ajwerner and a team June 1, 2021 03:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

One small thing, otherwise :lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @sajjadrizvi)


pkg/sql/gcjob/gc_job.go, line 86 at r1 (raw file):

	defer func() {
		if err != nil && !r.isPermanentGCError(err) {
			err = jobs.NewRetryJobError(err.Error())

these APIs are confusing, but I think it'd be better to do the following:

err = errors.Mark(err, jobs.NewRetryJobError("gc"))

This way we'll preserve the structure for logging rather than just redacting the entire string.

You can see this if you stick the following in a test in the jobs package:

func TestMarkWorks(t *testing.T) {
	err := errors.New("boom")
	marked := errors.Mark(err, NewRetryJobError("foo"))
	require.True(t, errors.Is(marked, retryJobErrorSentinel))
}

In the previous implementation, failed GC jobs were not being retried
regardless whether the failure is permanent or transient. As a
result, a GC job's failure risked orphaned data, which cannot be
reclaimed.

This commit adds a mechanism to retry failed GC jobs that are not
permanent. No limit is set on the number of retries. For the time
being, the failure type is determined based on the failure
categorization of schema-change jobs. This behavior is expected to
change once exponential backoff mechanism is implemented for failed
jobs (cockroachdb#44594).

Release note: None

Fixes: cockroachdb#65000
Copy link
Author

@sajjadrizvi sajjadrizvi left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)


pkg/sql/gcjob/gc_job.go, line 86 at r1 (raw file):

Previously, ajwerner wrote…

these APIs are confusing, but I think it'd be better to do the following:

err = errors.Mark(err, jobs.NewRetryJobError("gc"))

This way we'll preserve the structure for logging rather than just redacting the entire string.

You can see this if you stick the following in a test in the jobs package:

func TestMarkWorks(t *testing.T) {
	err := errors.New("boom")
	marked := errors.Mark(err, NewRetryJobError("foo"))
	require.True(t, errors.Is(marked, retryJobErrorSentinel))
}

Got it! thanks for pointing it out. I have updated the code.

@sajjadrizvi
Copy link
Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Jun 1, 2021

Build succeeded:

@craig craig bot merged commit f5b21cf into cockroachdb:master Jun 1, 2021
sajjadrizvi pushed a commit to sajjadrizvi/cockroach that referenced this pull request Jun 1, 2021
In the previous implementation, failed GC jobs were not being retried
regardless whether the failure is permanent or transient. As a
result, a GC job's failure risked orphaned data, which cannot be
reclaimed.

This patch adds a mechanism to retry failed GC jobs that are not
permanent. No limit is set on the number of retries. For the time
being, the failure type is determined based on the failure
categorization of schema-change jobs. This behavior is expected to
change once exponential backoff mechanism is implemented for failed
jobs (cockroachdb#44594).

This is a backport of cockroachdb#65910.

Release note: None

Fixes: cockroachdb#65000
sajjadrizvi pushed a commit to sajjadrizvi/cockroach that referenced this pull request Jun 2, 2021
In the previous implementation, failed GC jobs were not being retried
regardless whether the failure is permanent or transient. As a
result, a GC job's failure risked orphaned data, which cannot be
reclaimed.

This patch adds a mechanism to retry failed GC jobs that are not
permanent. No limit is set on the number of retries. For the time
being, the failure type is determined based on the failure
categorization of schema-change jobs. This behavior is expected to
change once exponential backoff mechanism is implemented for failed
jobs (cockroachdb#44594).

This is a backport of cockroachdb#65910.

Release note: None

Fixes: cockroachdb#65000
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: GC job may fail with error which should be retried
3 participants