Replies: 8 comments 15 replies
-
|
Any Comments are welcomed! /cc @shizhMSFT @FeynmanZhou |
Beta Was this translation helpful? Give feedback.
-
|
/cc @Wwwsylvia @qweeah for comments |
Beta Was this translation helpful? Give feedback.
-
|
Hi @souleb, thanks for the design proposal!
|
Beta Was this translation helpful? Give feedback.
-
|
Regarding go-retryablehttp my proposition is to borrow the design, by having the same methods signature for I think a statement referencing go-retryablehttp in the source code above those methods shall be enough. |
Beta Was this translation helpful? Give feedback.
-
|
@souleb The go-retryablehttp package looks great for HTTP level retry. Applying the retryable http is also very simple by using retryClient.StandardClient(): retryClient := retryablehttp.NewClient()
retryClient.RetryMax = 10
authClient := &auth.Client{
Client: retryClient.StandardClient()
}
// use `authClient` for registry accessI don't think we need to re-invent the wheel in For example, type Repository struct {
// ...
// Retry specifies a backoff strategy. No retry is applied if nil.
Retry func(ctx context.Context) BackOff
}
type BackOff interface {
// Next returns the next backoff duration and whether to stop
Next() (time.Duration, bool)
}or even integrate with cenkalti/backoff, which is in MIT license. For most operations, we can just retry. |
Beta Was this translation helpful? Give feedback.
-
|
I agree with using go-retryablehttp. It is in the for the application level retry we could have: // given an error, a method i.e. Push or Fetch and the content, this function returns a boolean stating whether we can retry
type IsRetryable func(error ErrorCode, method string, content io.Reader) boolWhen coupling this with a |
Beta Was this translation helpful? Give feedback.
-
|
Looks good. |
Beta Was this translation helpful? Give feedback.
-
|
Great! Do we want to support the same
|
Beta Was this translation helpful? Give feedback.
-
|
an example based on the discussion so far: |
Beta Was this translation helpful? Give feedback.
-
|
It seems that everything in this example can be done at the HTTP client level. |
Beta Was this translation helpful? Give feedback.
-
|
Now the question becomes... Should |
Beta Was this translation helpful? Give feedback.
-
Retry for ORAS
During the remote access, it is not uncommon to have unstable network connections or temporarily unavailable / faulty cloud servers. Thus a retry pattern can be applied to mitigate this kind of issues. However, if the unexpected incident of the remote server takes a longer time to be mitigated, an opposite circuit breaker pattern should be applied. Since a suitable pattern should be chosen and applied according to the application specific business logic, Considering the scenarios of fetching regular layers and foreign layers, it is better to implement retry at the HTTP level instead of the However, users may not know such packages or they have concerns on the licenses. It might still be good to have a Related Projects
ProposalThis comment proposes a new package The retry logic should be implemented to at the package retry
type Transport struct {
// default to http.DefaultTransport if nil
Base http.RoundTripper
// we can export them in future versions of oras-go with better refactoring
backoff time.Duration
factor int
attempts int
jitter float64
}
var DefaultTransport = &Transport{
backoff: 250 * time.Millisecond,
factor: 2,
attempts: 5,
jitter: 0.1,
}
var DefaultClient = &http.Client{
Transport: DefaultTransport,
}
// NewTransport creates an HTTP Transport with the default retry policy
func NewTransport(base http.RoundTripper) *Transport
// NewClient creates an HTTP client with the default retry policy
func NewClient() *http.Clientto be developer friendly. |
Beta Was this translation helpful? Give feedback.
-
|
We can document the hint to use |
Beta Was this translation helpful? Give feedback.
-
|
Here's what I end up with: var DefaultClient = &http.Client{
Transport: &Transport{},
}
var DefaultPolicy Policy = &GenericPolicy{
Retryable: DefaultPredicate,
Backoff: DefaultBackoff,
RetryWaitMin: 200 * time.Millisecond, // can be named as MaxWait
RetryWaitMax: 3 * time.Second, // can be named as MinWait
RetryMax: 5, // can be named as MaxRetry
}
var DefaultPredicate Predicate = func(ctx context.Context, resp *http.Response, err error) (bool, error) {
// not implemented
}
var DefaultBackoff Backoff = func(attempt int, resp *http.Response) time.Duration {
// check Retry-After
// do linear backoff
backoff := 250 * time.Millisecond
factor := 2
jitter := 0.1
// not implemented
}
type Policy interface {
Retry(ctx context.Context, attempt int, resp *http.Response, err error) (time.Duration, error)
}
type Predicate func(ctx context.Context, resp *http.Response, err error) (bool, error)
type Backoff func(attempt int, resp *http.Response) time.Duration
type GenericPolicy struct {
Retryable Predicate
Backoff Backoff
RetryWaitMin time.Duration
RetryWaitMax time.Duration
RetryMax int
}
func (p *GenericPolicy) Retry(ctx context.Context, attempt int, resp *http.Response, err error) (time.Duration, error) {
if attempt > p.RetryMax {
return -1, err
}
if ok, err := p.Retryable(ctx, resp, err); !ok {
return -1, err
}
backoff := p.Backoff(attempt, resp)
if backoff < p.RetryWaitMin {
backoff = p.RetryWaitMin
}
if backoff > p.RetryWaitMax {
backoff = p.RetryWaitMax
}
return backoff, nil
}
type Transport struct {
// default to http.DefaultTransport if nil
Base http.RoundTripper
// detailed policy implementation
// factory pattern to allow the policy to store states per request
Policy func() Policy
}
func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
ctx := req.Context()
policy := t.Policy()
attempt := 0
for {
attempt++
resp, respErr := t.Base.RoundTrip(req)
duration, err := policy.Retry(ctx, attempt, resp, respErr)
if duration < 0 {
return resp, err
}
if respErr == nil {
resp.Body.Close()
}
timer := time.NewTimer(duration)
select {
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
case <-timer.C:
}
}
}
// NewTransport creates an HTTP Transport with the default retry policy
func NewTransport(base http.RoundTripper) *Transport {
return &Transport{
Base: base,
}
}
// NewClient creates an HTTP client with the default retry policy
func NewClient() *http.Client {
return &http.Client{
Transport: NewTransport(nil),
}
} |
Beta Was this translation helpful? Give feedback.
-
|
Hello @shizhMSFT, happy new year. In your last proposal, you stated:
I agree that providing docs on how to hook Coupling var DefaultPolicy Policy = &GenericPolicy{
Retryable: DefaultPredicate,
Backoff: DefaultBackoff,
RetryWaitMin: 200 * time.Millisecond, // can be named as MaxWait
RetryWaitMax: 3 * time.Second, // can be named as MinWait
RetryMax: 5, // can be named as MaxRetry
}might be just that. What about the licensing? I think MPL can be used with Apache license. |
Beta Was this translation helpful? Give feedback.
-
|
@souleb Happy New Year 🎉
That's exactly what I am thinking about. In For licensing, MPL can work with Apache license but not always, depending on the users' scenarios. Here's the plan / proposal: we will create a package // client.go
var DefaultClient = NewClient()
// NewClient creates an HTTP client with the default retry policy
func NewClient() *http.Client {
return &http.Client{
Transport: NewTransport(nil),
}
}
type Transport struct {
// default to http.DefaultTransport if nil
Base http.RoundTripper
// detailed policy implementation
// factory pattern to allow the policy to store states per request
Policy func() Policy
}
// NewTransport creates an HTTP Transport with the default retry policy
func NewTransport(base http.RoundTripper) *Transport {
return &Transport{
Base: base,
}
}
func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
ctx := req.Context()
policy := t.policy()
attempt := 0
for {
attempt++
resp, respErr := t.Base.RoundTrip(req)
duration, err := policy.Retry(ctx, attempt, resp, respErr)
if duration < 0 {
return resp, err
}
if respErr == nil {
resp.Body.Close()
}
timer := time.NewTimer(duration)
select {
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
case <-timer.C:
}
}
}
func (t *Transport) policy() Policy {
if t.Policy == nil {
return DefaultPolicy
}
return t.Policy()
}and // policy.go
var DefaultPolicy Policy = &GenericPolicy{
Retryable: DefaultPredicate,
Backoff: DefaultBackoff,
MinWait: 200 * time.Millisecond,
MaxWait: 3 * time.Second,
MaxRetry: 5,
}
var DefaultPredicate Predicate = func(ctx context.Context, resp *http.Response, err error) (bool, error) {
panic("not implemented")
}
var DefaultBackoff Backoff = LinearBackoff(250*time.Millisecond, 2, 0.1)
type Policy interface {
Retry(ctx context.Context, attempt int, resp *http.Response, err error) (time.Duration, error)
}
type Predicate func(ctx context.Context, resp *http.Response, err error) (bool, error)
type Backoff func(attempt int, resp *http.Response) time.Duration
func LinearBackoff(backoff time.Duration, factor int, jitter float64) Backoff {
return func(attempt int, resp *http.Response) time.Duration {
// check Retry-After
// do linear backoff with jitter
panic("not implemented")
}
}
type GenericPolicy struct {
Retryable Predicate
Backoff Backoff
MinWait time.Duration
MaxWait time.Duration
MaxRetry int
}
func (p *GenericPolicy) Retry(ctx context.Context, attempt int, resp *http.Response, err error) (time.Duration, error) {
if attempt > p.MaxRetry {
return -1, err
}
if ok, err := p.Retryable(ctx, resp, err); !ok {
return -1, err
}
backoff := p.Backoff(attempt, resp)
if backoff < p.MinWait {
backoff = p.MinWait
}
if backoff > p.MaxWait {
backoff = p.MaxWait
}
return backoff, nil
}We also want to include this |
Beta Was this translation helpful? Give feedback.
-
|
That's good to me. |
Beta Was this translation helpful? Give feedback.
-
|
I’d like to express support for implementation of this. Project Ratify relies upon ORAS-go for multiple simultaneous GET operations to the registry. deislabs/ratify#502 |
Beta Was this translation helpful? Give feedback.
-
Hello All,
I would like to open the discussion on design HTTP retries as of #147.
Proposal
Add HTTP Level retry with exponential backoff to the remote package.
Note: Only GET and HEAD requests can be retried. Retrying PUT/PATCH/POST requests is problematic.
For uploading the blob, the POST request is retriable but PATCH and PUT are not.
Note: The proposed API is based on the go-retryablehttp api.
We propose adding 2 new types:
CheckRetryis called after each request. If it returns false, the Client stopsretrying and returns the response to the caller. If it returns an error, that error
value is returned in lieu of the error from the request. The Client will close any
response body when retrying, but if the retry is aborted it is up to the
CheckRetry callback to properly close any response body before returning.
Backoffis called after a failing request to determine the amount of time thatshould pass before trying again.
RetryWaitMinandRetryWaitMaxare the minimum and maximum amount of time to wait for a retry,and are used to determine the backoff time.
RetryMaxis the maximum number of times to retry.Users can implement their own
CheckRetryandBackofffunctions to customizethe retry behavior.
We propose adding the following default functions to the Client:
The
rateLimitBackofffunction is used when we are rate limited and is implemented usinggolang.org/x/time/rate. The
RateLimit-Limitheader is retrieved from the response header and is used to initialize the rate limit bucket.
The DefaultBackoff function is used when we are not rate limited and the proposal is to port
polly-contrib WaitAndRetry with jitter code.
Design considerations
We should be able to retry all HTTP requests.
The response body should be read and closed before retrying, so that we can reuse the connection.
If we rewind the request body before each retry, we should be able to retry the PUT/PATCH/POST request as well.
A good example is the net/http package rewindBody.
Alternatives
Use the go-retryablehttp and provide
a custom
CheckRetryandBackofffunctions.Beta Was this translation helpful? Give feedback.
All reactions