diff --git a/server/backends/invocationdb/invocationdb.go b/server/backends/invocationdb/invocationdb.go index f0f93f0a43d..20cfb63471e 100644 --- a/server/backends/invocationdb/invocationdb.go +++ b/server/backends/invocationdb/invocationdb.go @@ -111,15 +111,14 @@ func (d *InvocationDB) CreateInvocation(ctx context.Context, ti *tables.Invocati func (d *InvocationDB) UpdateInvocation(ctx context.Context, ti *tables.Invocation) (bool, error) { updated := false var err error - retryOptions := &retry.Options{InitialBackoff: time.Second, Multiplier: 2.0, MaxRetries: 5} - for i, r := 1, retry.New(ctx, retryOptions); r.Next(); i++ { + for r := retry.DefaultWithContext(ctx); r.Next(); { err = d.h.TransactionWithOptions(ctx, db.Opts().WithQueryName("update_invocation"), func(tx *db.DB) error { result := tx.Where("`invocation_id` = ? AND `attempt` = ?", ti.InvocationID, ti.Attempt).Updates(ti) updated = result.RowsAffected > 0 return result.Error }) if d.h.IsDeadlockError(err) { - log.Warningf("Encountered deadlock when attempting to update invocation table for invocation %s, attempt %d of %d", ti.InvocationID, i, retryOptions.MaxRetries) + log.Warningf("Encountered deadlock when attempting to update invocation table for invocation %s, attempt %d of %d", ti.InvocationID, r.AttemptNumber(), r.MaxAttempts()) continue } break diff --git a/server/util/retry/BUILD b/server/util/retry/BUILD index 2ed7950a86d..10c988eee7b 100644 --- a/server/util/retry/BUILD +++ b/server/util/retry/BUILD @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "retry", @@ -6,3 +6,12 @@ go_library( importpath = "github.com/buildbuddy-io/buildbuddy/server/util/retry", visibility = ["//visibility:public"], ) + +go_test( + name = "retry_test", + srcs = ["retry_test.go"], + deps = [ + ":retry", + "@com_github_stretchr_testify//assert", + ], +) diff --git a/server/util/retry/retry.go b/server/util/retry/retry.go index c9d06b0f7cc..c10b2b948bc 100644 --- a/server/util/retry/retry.go +++ b/server/util/retry/retry.go @@ -12,7 +12,7 @@ type Options struct { InitialBackoff time.Duration // How long to wait after the first request MaxBackoff time.Duration // Max amount of time to wait for a single request Multiplier float64 // Next backoff is this * previous backoff - MaxRetries int // Max number of retries; 0 is inf + MaxRetries int // Max number of retries; 0 is based on time } type Retry struct { @@ -20,13 +20,28 @@ type Retry struct { ctx context.Context currentAttempt int + maxAttempts int isReset bool } func New(ctx context.Context, opts *Options) *Retry { + maxAttempts := opts.MaxRetries + if maxAttempts <= 0 { + // always try at least once + maxAttempts = 1 + if opts.Multiplier > 1 && opts.MaxBackoff > opts.InitialBackoff { + maxAttempts = 1 + int(math.Ceil( + math.Log( + float64(opts.MaxBackoff)/float64(opts.InitialBackoff), + )/math.Log(opts.Multiplier), + )) + } + } + r := &Retry{ - ctx: ctx, - opts: opts, + ctx: ctx, + opts: opts, + maxAttempts: maxAttempts, } r.Reset() return r @@ -40,17 +55,15 @@ func New(ctx context.Context, opts *Options) *Retry { // doSomething() // } func DefaultWithContext(ctx context.Context) *Retry { - r := &Retry{ - ctx: ctx, - opts: &Options{ + return New( + ctx, + &Options{ InitialBackoff: 50 * time.Millisecond, MaxBackoff: 3 * time.Second, Multiplier: 2, MaxRetries: 0, // unlimited, time based max by default. }, - } - r.Reset() - return r + ) } func (r *Retry) Reset() { @@ -74,7 +87,7 @@ func (r *Retry) Next() bool { } // If we're out of retries, exit. - if r.opts.MaxRetries > 0 && r.currentAttempt == r.opts.MaxRetries { + if r.currentAttempt >= r.maxAttempts { return false } @@ -86,3 +99,11 @@ func (r *Retry) Next() bool { return false } } + +func (r *Retry) AttemptNumber() int { + return r.currentAttempt + 1 +} + +func (r *Retry) MaxAttempts() int { + return r.maxAttempts +} diff --git a/server/util/retry/retry_test.go b/server/util/retry/retry_test.go new file mode 100644 index 00000000000..b3c89c310c3 --- /dev/null +++ b/server/util/retry/retry_test.go @@ -0,0 +1,85 @@ +package retry_test + +import ( + "context" + "testing" + + "github.com/buildbuddy-io/buildbuddy/server/util/retry" + "github.com/stretchr/testify/assert" +) + +func TestMaxRetryFromBackoff(t *testing.T) { + tests := map[string]struct { + inputOptions *retry.Options + expectedMaxAttempts int + }{ + "exact backoff boundary": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 24, + Multiplier: 2, + MaxRetries: 0, + }, + expectedMaxAttempts: 4, + }, + "above exact backoff boundary": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 25, + Multiplier: 2, + MaxRetries: 0, + }, + expectedMaxAttempts: 5, + }, + "below exact backoff boundary": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 23, + Multiplier: 2, + MaxRetries: 0, + }, + expectedMaxAttempts: 4, + }, + "manually set maximum above calculated backoff": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 23, + Multiplier: 2, + MaxRetries: 7, + }, + expectedMaxAttempts: 7, + }, + "manually set maximum below calculated backoff": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 23, + Multiplier: 2, + MaxRetries: 2, + }, + expectedMaxAttempts: 2, + }, + "initial backoff greater than maximum": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 1, + Multiplier: 2, + MaxRetries: 0, + }, + expectedMaxAttempts: 1, + }, + "multiplier less than or equal to one": { + inputOptions: &retry.Options{ + InitialBackoff: 3, + MaxBackoff: 7, + Multiplier: 0.5, + MaxRetries: 0, + }, + expectedMaxAttempts: 1, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.expectedMaxAttempts, retry.New(context.Background(), tc.inputOptions).MaxAttempts()) + }) + } +}