-
Notifications
You must be signed in to change notification settings - Fork 0
Automatic Retry with Exponential Backoff #91
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
|
|
||
| "github.com/Snider/Borg/cmd" | ||
| "github.com/Snider/Borg/pkg/logger" | ||
| "github.com/Snider/Borg/pkg/retry" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| ) | ||
|
|
||
| var osExit = os.Exit | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| package retry | ||
|
|
||
| import ( | ||
| "math/rand" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The func init() {
rand.Seed(time.Now().UnixNano())
} |
||
| "net/http" | ||
| "time" | ||
| ) | ||
|
|
||
| // Backoff is a time.Duration that represents the backoff strategy. | ||
| type Backoff time.Duration | ||
|
|
||
| // Exponential returns a new Backoff duration that is the current duration | ||
| // multiplied by 2. | ||
| func (b Backoff) Exponential() Backoff { | ||
| return Backoff(time.Duration(b) * 2) | ||
| } | ||
|
|
||
| // Jitter returns a new Backoff duration with a random jitter added. | ||
| func (b Backoff) Jitter(factor float64) Backoff { | ||
| if factor <= 0 { | ||
| return b | ||
| } | ||
| jitter := time.Duration(rand.Float64() * factor * float64(b)) | ||
| return Backoff(time.Duration(b) + jitter) | ||
| } | ||
|
|
||
| // Transport is an http.RoundTripper that automatically retries requests. | ||
| type Transport struct { | ||
| // Transport is the underlying http.RoundTripper to use for requests. | ||
| // If nil, http.DefaultTransport is used. | ||
| Transport http.RoundTripper | ||
|
|
||
| // Retries is the maximum number of retries to attempt. | ||
| Retries int | ||
|
|
||
| // InitialBackoff is the initial backoff duration. | ||
| InitialBackoff time.Duration | ||
|
|
||
| // MaxBackoff is the maximum backoff duration. | ||
| MaxBackoff time.Duration | ||
|
|
||
| // Jitter is the jitter factor to apply to backoff durations. | ||
| Jitter float64 | ||
| } | ||
|
|
||
| // NewTransport creates a new Transport with default values. | ||
| func NewTransport() *Transport { | ||
| return &Transport{ | ||
| Transport: http.DefaultTransport, | ||
| Retries: 3, | ||
| InitialBackoff: 1 * time.Second, | ||
| MaxBackoff: 30 * time.Second, | ||
| Jitter: 0.1, | ||
| } | ||
| } | ||
|
|
||
| // RoundTrip implements the http.RoundTripper interface. | ||
| func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| var resp *http.Response | ||
| var err error | ||
| var backoff = Backoff(t.InitialBackoff) | ||
|
|
||
| for i := 0; i < t.Retries; i++ { | ||
| resp, err = t.transport().RoundTrip(req) | ||
| if err == nil && resp.StatusCode < 500 { | ||
| return resp, err | ||
| } | ||
|
|
||
| if i < t.Retries-1 { | ||
| time.Sleep(time.Duration(backoff)) | ||
| backoff = backoff.Exponential().Jitter(t.Jitter) | ||
| if backoff > Backoff(t.MaxBackoff) { | ||
| backoff = Backoff(t.MaxBackoff) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+63
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a critical issue in the retry logic. When attempts := t.Retries
if attempts <= 0 {
attempts = 1
}
for i := 0; i < attempts; i++ {
resp, err = t.transport().RoundTrip(req)
if err == nil && resp.StatusCode < 500 {
return resp, err
}
if i < attempts-1 {
time.Sleep(time.Duration(backoff))
backoff = backoff.Exponential().Jitter(t.Jitter)
if backoff > Backoff(t.MaxBackoff) {
backoff = Backoff(t.MaxBackoff)
}
}
} |
||
| return resp, err | ||
| } | ||
|
|
||
| func (t *Transport) transport() http.RoundTripper { | ||
| if t.Transport != nil { | ||
| return t.Transport | ||
| } | ||
| return http.DefaultTransport | ||
| } | ||
|
|
||
| // NewClient returns a new http.Client that uses the Transport. | ||
| func NewClient(transport *Transport) *http.Client { | ||
| return &http.Client{ | ||
| Transport: transport, | ||
| } | ||
| } | ||
|
|
||
| var ( | ||
| // DefaultTransport is the default transport with retry logic. | ||
| DefaultTransport = NewTransport() | ||
| // DefaultClient is the default client that uses the default transport. | ||
| DefaultClient = NewClient(DefaultTransport) | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package retry | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| func TestTransport_RoundTrip(t *testing.T) { | ||
| var requestCount int | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| requestCount++ | ||
| if requestCount <= 2 { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| transport := NewTransport() | ||
| transport.Retries = 3 | ||
| transport.InitialBackoff = 1 * time.Millisecond | ||
| transport.MaxBackoff = 10 * time.Millisecond | ||
|
|
||
| client := NewClient(transport) | ||
| req, _ := http.NewRequest("GET", server.URL, nil) | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| t.Errorf("expected status code %d, got %d", http.StatusOK, resp.StatusCode) | ||
| } | ||
|
|
||
| if requestCount != 3 { | ||
| t.Errorf("expected 3 requests, got %d", requestCount) | ||
| } | ||
| } | ||
|
Comment on lines
+10
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test for
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "time" | ||
|
|
||
| "github.com/Snider/Borg/pkg/datanode" | ||
|
|
||
|
|
@@ -37,12 +38,37 @@ func (g *gitCloner) CloneGitRepository(repoURL string, progress io.Writer) (*dat | |
| cloneOptions.Progress = progress | ||
| } | ||
|
|
||
| _, err = git.PlainClone(tempPath, false, cloneOptions) | ||
| if err != nil { | ||
| var lastErr error | ||
| retries := 3 | ||
| backoff := 1 * time.Second | ||
| maxBackoff := 30 * time.Second | ||
|
|
||
| for i := 0; i < retries; i++ { | ||
| _, err = git.PlainClone(tempPath, false, cloneOptions) | ||
| if err == nil { | ||
| lastErr = nil | ||
| break | ||
| } | ||
|
|
||
| lastErr = err | ||
|
|
||
| // Handle non-retryable error | ||
| if err.Error() == "remote repository is empty" { | ||
| return datanode.New(), nil | ||
| } | ||
| return nil, err | ||
|
|
||
| // Don't wait on the last attempt | ||
| if i < retries-1 { | ||
| time.Sleep(backoff) | ||
| backoff *= 2 | ||
| if backoff > maxBackoff { | ||
| backoff = maxBackoff | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if lastErr != nil { | ||
| return nil, lastErr | ||
| } | ||
|
Comment on lines
+41
to
72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file introduces a manual implementation of a retry loop with exponential backoff. Since the pull request's main purpose is to add a centralized |
||
|
|
||
| dn := datanode.New() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring errors from flag parsing can lead to silent failures and unexpected behavior if a flag is ever renamed or removed. For better robustness, these errors should be handled. You can switch from
PersistentPreRuntoPersistentPreRunE, which allows you to return an error thatcobrawill then handle and display to the user.