add support for HTTP retries #436

lionelvillard · 2020-04-02T20:25:15Z

Add support for retrying HTTP requests when a certain class of error occurs.

There is no tests yet. Opening the PR to get early feedback

n3wscott

Yeah I think this is the right track. Nice work!! Can you add some tests maybe in the v2/testing area ? This is pretty hard to test isolated unless you know some tricks.

v2/protocol/http/protocol.go

n3wscott · 2020-04-03T04:55:40Z

v2/protocol/http/retries_result.go

+	if e.Retries == 0 {
+		return e.Result.Error()
+	}
+	return fmt.Sprintf("%s (%dx)", e.Result.Error(), e.Retries)


This might do funky things with the ack wrapping so a test for this is needed

n3wscott · 2020-04-03T04:55:40Z

v2/protocol/http/retries_result.go

+	if e.Retries == 0 {
+		return e.Result.Error()
+	}
+	return fmt.Sprintf("%s (%dx)", e.Result.Error(), e.Retries)


This might do funky things with the ack wrapping so a test for this is needed

slinkydeveloper

I like it! Just two points:

Since WithRetry is an HTTP specific feature, should we move it into protocol/http? @n3wscott wdyt?
I think Backoff should not live in types, since it's the module that contains the cloudevents spec types, not the data structure about the logic of the client. Probably a better place for Backoff is in the same module of WithRetry or in client module

n3wscott · 2020-04-03T14:47:07Z

v2/protocol/http/retries_result.go

+	protocol.Result
+
+	// Retries is the number of times the request was tried
+	Retries int


I think we want to return back the total time spent trying too if we do exp backoff, what do you think?

to make sure we are on the same page, this is the time spent trying excluding the succesful (if any) request, right. I'm pretty sure the answer is yes...

n3wscott · 2020-04-03T14:47:47Z

v2/types/backoff.go

+import "time"
+
+// Backoff holds parameters applied to retries
+type Backoff struct {


Rather than make a custom type, I would hide that detail from the user with a change to the WithBackoff method, I will comment there.

n3wscott · 2020-04-03T14:48:51Z

v2/context/context.go

+}
+
+// RetryFrom looks in the given context and returns the backoff parameters if found, otherwise nil
+func RetryFrom(ctx context.Context) *types.Backoff {


I know slinky recommended moving this to http but there are settings in other protocols that have similar settings, I feel like it is ok to keep it in the general spot here.

Ok i'm fine with that

n3wscott · 2020-04-03T14:55:53Z

v2/context/context.go

+var retryKey = retryKeyType{}
+
+// WithRetry returns back a new context with the retry backoff parameters.
+func WithRetry(ctx context.Context, backoff types.Backoff) context.Context {


We might want more than one strategy for backoff, this is linear and we might also want exponential backoff.

Also, rather than a custom type (or an internal custom type is ok) I would not expose that to the user. I recommend being explicit about the backoff and retries:

func WithLinearBackoff(ctx context.Context, period time.Duration, maxTries uint) context.Context {

is one way. I think internally we should make a type, it can be defined here in this file:

type BackoffConfig struct { Strategy string Period time.Duration MaxRetries int }

And WithLinearBackoff makes a BackoffConfig{Strategy:"linear", ....

I suggest this to let the api self documenting and not require the caller to understand another object to make.

sound good. Fixing....

n3wscott · 2020-04-03T15:02:34Z

v2/protocol/http/protocol.go

+			retries++
+
+			// Linear backoff.
+			time.Sleep(backoff.Delay)


I would make this method more of a wrapper for doOnce with a some kind of pattern of:

for { select { case <-ctx.Done(): // canceled. case <-timer.Tick(): strongTypedErr: doOnce(ctx, request) ... custom logic to see if we should stop retry } }

That way the logic of producing the protocol.NewReceipt is only one place, and you are still free to collect them in the outer retry method.

I tried but the reason I didn't call doOnce is because of if retry && !err.(*url.Error).Timeout() {. When doOnce returns an error, there is no way to unwrap the original error.

which can be easily solved by changing doOnce signature.

ended up trying go 1.13 unwrap API. Not tested yet.

n3wscott · 2020-04-03T15:03:18Z

v2/protocol/http/protocol.go

@@ -133,6 +145,64 @@ func (p *Protocol) Request(ctx context.Context, m binding.Message) (binding.Mess
 	return NewMessage(resp.Header, resp.Body), NewResult(resp.StatusCode, "%w", result)
 }

+func (p *Protocol) doWithRetry(backoff *types.Backoff) func(*http.Request) (binding.Message, error) {


I think this needs context.Context passed in to allow for the app to cancel in the case of shutdown or term signals.

good point! thanks!

lionelvillard · 2020-04-03T21:05:13Z

this is ready for a second round of review. I added some tests under v2/protocol/http. I didn't see v2/test. I'm happy to move the tests over there if needed.

@n3wscott @slinkydeveloper

slinkydeveloper

Can you move Backoff outside types? I think the proper place is content module

lionelvillard · 2020-04-06T13:15:57Z

@slinkydeveloper there is no content package. Do you mean context?

slinkydeveloper · 2020-04-06T13:31:32Z

yep, context package

slinkydeveloper

LGTM, waiting for scott review

v2/context/context.go

n3wscott · 2020-04-06T15:24:34Z

v2/context/context.go

+// Opaque key type used to store linear backoff parameters
+type linearBackoffKeyType struct{}
+
+var linearBackoffKey = linearBackoffKeyType{}


this should just be a backoffKey

using retriesKey.

v2/context/context.go

n3wscott · 2020-04-06T15:28:05Z

v2/protocol/http/protocol.go

@@ -118,6 +118,16 @@ func (p *Protocol) Request(ctx context.Context, m binding.Message) (binding.Mess
 	if err = WriteRequest(ctx, m, req, p.transformers); err != nil {
 		return nil, err
 	}
+
+	do := p.doOnce
+	if delay, tries := cecontext.LinearBackoffFrom(ctx); tries > 0 {


I would change this method to BackoffFrom and return the Backoff struct, then have a switch for BackoffStrategyLinear or BackoffStrategyNone

v2/context/context.go

v2/context/backoff.go

Signed-off-by: Lionel Villard <villard@us.ibm.com>

n3wscott · 2020-04-06T18:04:42Z

LGTM!!

Thanks, this is gonna be awesome!!

n3wscott reviewed Apr 3, 2020

View reviewed changes

slinkydeveloper reviewed Apr 3, 2020

View reviewed changes

n3wscott reviewed Apr 3, 2020

View reviewed changes

lionelvillard force-pushed the retries branch from b01b97a to 9f2ad9c Compare April 3, 2020 21:01

lionelvillard changed the title ~~WIP: add support for HTTP retries~~ add support for HTTP retries Apr 3, 2020

slinkydeveloper suggested changes Apr 6, 2020

View reviewed changes

slinkydeveloper added component/protocol/http enhancement New feature or request SDKv2 labels Apr 6, 2020

slinkydeveloper added this to the SDK v2 milestone Apr 6, 2020

slinkydeveloper linked an issue Apr 6, 2020 that may be closed by this pull request

Add support for retries #423

Closed

lionelvillard force-pushed the retries branch from 230a749 to 4fc878b Compare April 6, 2020 13:36

slinkydeveloper self-requested a review April 6, 2020 13:41

slinkydeveloper approved these changes Apr 6, 2020

View reviewed changes

n3wscott reviewed Apr 6, 2020

View reviewed changes

v2/context/context.go Outdated Show resolved Hide resolved

n3wscott reviewed Apr 6, 2020

View reviewed changes

v2/context/context.go Show resolved Hide resolved

n3wscott reviewed Apr 6, 2020

View reviewed changes

v2/context/context.go Outdated Show resolved Hide resolved

n3wscott reviewed Apr 6, 2020

View reviewed changes

v2/context/backoff.go Outdated Show resolved Hide resolved

lionelvillard added 2 commits April 6, 2020 13:40

WIP: add support for HTTP retries

a9f5592

Signed-off-by: Lionel Villard <villard@us.ibm.com>

add retries duration. Cleaner API

4ad540f

Signed-off-by: Lionel Villard <villard@us.ibm.com>

lionelvillard added 5 commits April 6, 2020 13:42

bare minimum http retry test

6a48999

Signed-off-by: Lionel Villard <villard@us.ibm.com>

adding more tests

7008e42

Signed-off-by: Lionel Villard <villard@us.ibm.com>

more tests

f0e3a8e

Signed-off-by: Lionel Villard <villard@us.ibm.com>

move backoff under context

2f28c00

Signed-off-by: Lionel Villard <villard@us.ibm.com>

changes after code review

6c37e72

Signed-off-by: Lionel Villard <villard@us.ibm.com>

lionelvillard force-pushed the retries branch from 1bea401 to 6c37e72 Compare April 6, 2020 17:42

fix linting

1905c75

Signed-off-by: Lionel Villard <villard@us.ibm.com>

n3wscott merged commit abe3124 into cloudevents:master Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for HTTP retries #436

add support for HTTP retries #436

lionelvillard commented Apr 2, 2020

n3wscott left a comment

n3wscott Apr 3, 2020

n3wscott Apr 3, 2020

slinkydeveloper left a comment

n3wscott Apr 3, 2020

lionelvillard Apr 3, 2020

n3wscott Apr 3, 2020

n3wscott Apr 3, 2020

slinkydeveloper Apr 3, 2020

n3wscott Apr 3, 2020

lionelvillard Apr 3, 2020

n3wscott Apr 3, 2020

lionelvillard Apr 3, 2020

lionelvillard Apr 3, 2020

lionelvillard Apr 3, 2020

n3wscott Apr 3, 2020

lionelvillard Apr 3, 2020

lionelvillard commented Apr 3, 2020

slinkydeveloper left a comment

lionelvillard commented Apr 6, 2020

slinkydeveloper commented Apr 6, 2020

slinkydeveloper left a comment

n3wscott Apr 6, 2020

lionelvillard Apr 6, 2020

n3wscott Apr 6, 2020

n3wscott commented Apr 6, 2020

add support for HTTP retries #436

add support for HTTP retries #436

Conversation

lionelvillard commented Apr 2, 2020

n3wscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionelvillard commented Apr 3, 2020

slinkydeveloper left a comment

Choose a reason for hiding this comment

lionelvillard commented Apr 6, 2020

slinkydeveloper commented Apr 6, 2020

slinkydeveloper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3wscott commented Apr 6, 2020