Skip to content

Commit cf80108

Browse files
committed
changefeedccl: add retry to TestChangefeedFailOnTableOffline
Previously, this TestChangefeedFailOnTableOffline test could fail if the import attempt failed with "result is ambiguous". This error should be retried. This should reduce this source of flakiness for that test. Fixes: #142033 Release note: None
1 parent 2271e09 commit cf80108

File tree

4 files changed

+44
-1
lines changed

4 files changed

+44
-1
lines changed

pkg/ccl/changefeedccl/changefeed_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5195,7 +5195,7 @@ func TestChangefeedFailOnTableOffline(t *testing.T) {
51955195
// Start an import job which will immediately pause after ingestion
51965196
sqlDB.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = 'import.after_ingest';")
51975197
go func() {
5198-
sqlDB.ExpectErrWithTimeout(t, `pause point`, `IMPORT INTO for_import CSV DATA ($1);`, dataSrv.URL)
5198+
sqlDB.ExpectErrWithRetry(t, `pause point`, `IMPORT INTO for_import CSV DATA ($1);`, `result is ambiguous`, dataSrv.URL)
51995199
}()
52005200
sqlDB.CheckQueryResultsRetry(
52015201
t,

pkg/testutils/sqlutils/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ go_library(
2929
"//pkg/sql/sem/tree",
3030
"//pkg/testutils",
3131
"//pkg/util/protoutil",
32+
"//pkg/util/retry",
3233
"//pkg/util/timeutil",
3334
"@com_github_cockroachdb_cockroach_go_v2//crdb",
3435
"@com_github_cockroachdb_errors//:errors",

pkg/testutils/sqlutils/sql_runner.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616

1717
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
1818
"github.com/cockroachdb/cockroach/pkg/testutils"
19+
"github.com/cockroachdb/cockroach/pkg/util/retry"
1920
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
2021
"github.com/cockroachdb/errors"
2122
"github.com/lib/pq"
@@ -244,6 +245,24 @@ func (sr *SQLRunner) ExpectErrWithTimeout(
244245
}
245246
}
246247

248+
// ExpectErrWithRetry wraps ExpectErr with a timeout and retry on a specific error.
249+
func (sr *SQLRunner) ExpectErrWithRetry(
250+
t Fataler, errRE string, query string, retryableErrorRE string, args ...interface{},
251+
) {
252+
helperOrNoop(t)()
253+
var err error
254+
255+
retryOpts := retry.Options{MaxRetries: 5}
256+
err = retryOpts.DoWithRetryable(context.Background(), func(ctx context.Context) (bool, error) {
257+
_, err = sr.DB.ExecContext(context.Background(), query, args...)
258+
if testutils.IsError(err, retryableErrorRE) {
259+
return true, err
260+
}
261+
return false, err
262+
})
263+
sr.expectErr(t, query, err, errRE)
264+
}
265+
247266
// Query is a wrapper around gosql.Query that kills the test on error.
248267
func (sr *SQLRunner) Query(t Fataler, query string, args ...interface{}) *gosql.Rows {
249268
helperOrNoop(t)()

pkg/util/retry/retry.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,29 @@ func (opts Options) Do(ctx context.Context, fn func(ctx context.Context) error)
180180
return err
181181
}
182182

183+
// DoWithRetryable invokes the closure according to the retry options until it
184+
// returns success or a non-retryable error. Always returns an error unless the
185+
// return is prompted by a successful invocation of `fn`.
186+
func (opts Options) DoWithRetryable(
187+
ctx context.Context, fn func(ctx context.Context) (retryable bool, err error),
188+
) error {
189+
var err error
190+
for r := StartWithCtx(ctx, opts); r.Next(); {
191+
var retryable bool
192+
retryable, err = fn(ctx)
193+
if err == nil {
194+
return nil
195+
}
196+
if !retryable {
197+
return err
198+
}
199+
}
200+
if err == nil {
201+
return errors.AssertionFailedf("never invoked function in DoWithRetryable")
202+
}
203+
return err
204+
}
205+
183206
// WithMaxAttempts is a helper that runs fn N times and collects the last err.
184207
// The function will terminate early if the provided context is canceled, but it
185208
// guarantees that fn will run at least once.

0 commit comments

Comments
 (0)