Be smarter about retries

[embano1]

The dispatcher currently considers all !2xx || !3xx status codes as retryable, which is somewhat against the HTTP protocol spec:

https://github.com/knative-sandbox/eventing-rabbitmq/blob/928976461374266e0c029bc61719c68b3dffad8b/pkg/dispatcher/dispatcher.go#L160

I'm happy to file a PR if we get consensus on which codes we want to retry. Related work:

• https://docs.aws.amazon.com/apigateway/api-reference/handling-errors/
• https://github.com/hashicorp/go-retryablehttp/blob/master/client.go#L424

[xtreme-sameer-vohra]

Thanks for filing this @embano1
Seems pretty reasonable to be more specific. At minimum we should cover the explicit http status checks.

[benmoss]

https://github.com/knative/specs/blob/main/specs/eventing/data-plane.md#event-acknowledgement-and-delivery-retry

[benmoss]

https://github.com/knative-sandbox/eventing-kafka-broker/blob/3b860bf2870caa2b27e39683d7507fd6dd6cd136/data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/WebClientCloudEventSender.java#L84-L90

https://github.com/knative/eventing/blob/f031ba23b23d20ed8d13491000feb0abcf50d562/pkg/channel/message_dispatcher.go#L312-L315

It seems this might be the standard among implementations right now

[embano1]

I wonder why this is overwritten at all and not the default option used? https://github.com/cloudevents/sdk-go/blob/c6de9589bb3578364689adc64414000dabe93c01/v2/protocol/http/protocol.go#L40

This looks like it does what we need (although retrying on 500 would be nice too, I may want to file a PR for the SDK)? cloudevents/sdk-go#718

[benmoss]

Yeah, it definitely seems like something the sdk should be handling

[lionelvillard]

just in case you haven't seen this: https://github.com/knative/eventing/blob/main/pkg/kncloudevents/retries.go

and this: knative/eventing#5285

[embano1]

@lionelvillard are you suggesting we should be using kncloudevents as client?

[lionelvillard]

I'm just raising awareness, not suggesting anything :-) kncloudevents is meant to be Knative eventing compliant so maybe it makes sense for you to use it.

[embano1]

@benmoss thoughts?

[vaikas]

@embano1 for the context on why the overrides:
#113

And:
cloudevents/sdk-go#595

Not saying it's correct the way it is, just answering the why :)

[embano1]

Not saying it's correct the way it is, just answering the why :)

Gotcha! So IMHO we have couple of options here (most, if not all of them breaking):

1. adopt kncloudevents client
2. drop custom retryer in favor of default in ce-client
3. enhance custom retryer as proposed above

[benmoss]

I'm confused what the desired behavior even is. We have a bunch of different definitions of when to retry and not, but not much clarity on which one is correct. I think I'd be most comfortable for us to follow the the eventing spec, but happy to hear arguments for alternative views.

[embano1]

I agree Ben, we should align with the Knative Eventing spec then - even though I'm not 100% on the same page with some decisions here.

But this also means we have to implement our own retry code mapping [1] since the CloudEvents SDK has a diverging (from the above spec) set of defaults:

var defaultRetriableErrors = map[int]string{
	404: "Not Found",
	413: "Payload Too Large",
	425: "Too Early",
	429: "Too Many Requests",
	502: "Bad Gateway",
	503: "Service Unavailable",
	504: "Gateway Timeout",
}

[1]
Not sure if there's already a pkg in Knative land which provides these mappings in compliance with the SPEC?