From 3ba807b5fb307fb30f7d7f8bab1850ecbb6157d1 Mon Sep 17 00:00:00 2001 From: Mark Hindess Date: Sun, 23 Jul 2023 13:34:52 +0100 Subject: [PATCH 1/2] fix: admin retry logic The previous fix wasn't correct. While looking at the transaction_manager.go logic, it became clear that the intention is that retry.max should not count the first attempt. This seems very reasonable so this fixes the test and logic to reflect that. The original code would fail this updated test because it didn't make the final required attempt. Signed-off-by: Mark Hindess --- admin.go | 2 +- admin_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/admin.go b/admin.go index b04102e2f..d64652c2d 100644 --- a/admin.go +++ b/admin.go @@ -207,7 +207,7 @@ func isErrNoController(err error) bool { // provided retryable func) up to the maximum number of tries permitted by // the admin client configuration func (ca *clusterAdmin) retryOnError(retryable func(error) bool, fn func() error) error { - for attemptsRemaining := ca.conf.Admin.Retry.Max; ; { + for attemptsRemaining := ca.conf.Admin.Retry.Max + 1; ; { err := fn() attemptsRemaining-- if err == nil || attemptsRemaining == 0 || !retryable(err) { diff --git a/admin_test.go b/admin_test.go index ea7519935..0728091bd 100644 --- a/admin_test.go +++ b/admin_test.go @@ -1852,11 +1852,11 @@ func Test_retryOnError(t *testing.T) { if err == nil { t.Errorf("expected error but was nil") } - if attempts != 3 { - t.Errorf("expected 3 attempts to have been made but was %d", attempts) + if attempts != 4 { + t.Errorf("expected 4 attempts to have been made but was %d", attempts) } - if time.Since(startTime) >= 3*testBackoffTime { - t.Errorf("attempt+sleep+attempt+sleep+attempt should take less than 3 * backoff time") + if time.Since(startTime) >= 4*testBackoffTime { + t.Errorf("attempt+sleep+retry+sleep+retry+sleep+retry should take less than 4 * backoff time") } }) } From df58534c17b37febcf3ccb66655378ea8fda589d Mon Sep 17 00:00:00 2001 From: Mark Hindess Date: Sun, 23 Jul 2023 13:55:07 +0100 Subject: [PATCH 2/2] fix: use safer condition Without the previous fix, Retry.Max == 0 would potentially loop on retryable errors. This is bad so even though it shouldn't happen now. Make the condition safer. Signed-off-by: Mark Hindess --- admin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admin.go b/admin.go index d64652c2d..03b5438be 100644 --- a/admin.go +++ b/admin.go @@ -210,7 +210,7 @@ func (ca *clusterAdmin) retryOnError(retryable func(error) bool, fn func() error for attemptsRemaining := ca.conf.Admin.Retry.Max + 1; ; { err := fn() attemptsRemaining-- - if err == nil || attemptsRemaining == 0 || !retryable(err) { + if err == nil || attemptsRemaining <= 0 || !retryable(err) { return err } Logger.Printf(