Initial error handling support by lionelvillard · Pull Request #1949 · knative/eventing

lionelvillard · 2019-09-23T17:30:54Z

Helps #1573 and #1493

Proposed Changes

Add DeliverySpec and DeliveryStatus types
Add DeliverySpec to Channelable
Add DeliveryStatus to ChannelableStatus

Baby step towards proper error handling.
Design document: https://docs.google.com/document/d/1qRrzGoHJQO-oc5p-yRK8IRfugd-FM_PXyM7lN5kcqks/edit#

lionelvillard · 2019-09-23T17:31:38Z

/hold

Holding while the design is under review.

n3wscott · 2019-09-25T15:32:08Z

+
+### Dead-Letter Channel
+
+Channel implementations might leverage the existing native error handling support they provide, usually a dead letter channel, to forward failed messages to the error sink. In that case, the error sink might be realized by creating a subscription on the error channel.


Is the plan still to expose this channel as a channel in the status?

yes only if the channel supports it.

matzew · 2019-09-26T09:18:01Z

+    * to channel subscribers.
+    * to source sink.
+    * to broker/triggers.
+* Be able to identify a message couldn’t be delivered (Observability)


s/identify a message/identify messages ?

matzew · 2019-09-26T09:18:57Z

+    * to source sink.
+    * to broker/triggers.
+* Be able to identify a message couldn’t be delivered (Observability)
+* Be able to leverage existing native error handling mechanisms (eg. dead letter queues).


native: could we be more specific here? that native means "platform native", if available or the like ?

matzew · 2019-09-26T09:19:57Z

+    * to broker/triggers.
+* Be able to identify a message couldn’t be delivered (Observability)
+* Be able to leverage existing native error handling mechanisms (eg. dead letter queues).
+* Be able to redirect of error'ed events from a channel.


events? or messages? let's be consistent, and I think event is the right term, over message(s) /cc @nachocano

I'm fine with using event everywhere.

matzew · 2019-09-26T10:19:05Z

+
+### Dead-Letter Channel
+
+Channel implementations might leverage the existing native error handling support they provide, usually a dead letter channel, to forward failed messages to the error sink. In that case, the error sink might be realized by creating a subscription on the error channel.


Channel implementations might leverage the existing native error handling support they provide, usually a dead letter channel

Could we add a link to the EIP definition?

and perhaps add something like

Knative Channel implementations may leverage existing platform native error handling support they might provide, like a a [_Dead Letter Channel_](https://www.enterpriseintegrationpatterns.com/patterns/messaging/DeadLetterChannel.html), to forward failed messages from their _Dead Letter Channel_ to the configured error sink.

matzew · 2019-09-26T10:26:16Z

+
+Typically channel implementations and event sources retry sending messages before redirecting them to the error sink.
+While there are many different ways to implement the retry logic
+(immediate retry, retry queue, etc...), implementations usually


we want to somewhat categories / spec this ? Right now, it's not really know if channels/sources do that (and how often by default)

Maybe changing the wording would help.

Channel implementations and event sources should retry ...

?

matzew · 2019-09-26T10:29:47Z

+
+### Delivery Specification
+
+The goal of this delivery specification is to formally define the vocabulary related to capabilities defined above (error sink, dead-letter queues and retry) to provide consistency across all Knative event sources, channels and brokers.


Knative event sources, channels implementations and brokers

?

matzew · 2019-09-26T10:32:27Z

+)
+```
+
+Channel, brokers and event sources  are not required to support all this capabilities and are free to add more delivery options.


Channel implementations, brokers and ....

matzew · 2019-09-26T10:33:05Z

+
+### Exposing underlying DLC
+
+Channels supporting dead letter queue should advertise it in their status.


we mix wording here. DLC / DLQ

EIP calls it "Dead Letter Channel", let's stay with that ?

lionelvillard · 2019-10-07T13:06:06Z

/hold cancel
/assign @vaikas-google

vaikas

Thanks for whipping this into shape! Couple of comments.

vaikas · 2019-10-09T23:17:13Z

+	// More information on Duration format: https://www.ietf.org/rfc/rfc3339.txt
+	//
+	// For linear policy, backoff delay is the time interval between retries.
+	// For exponential policy , backoff delay is backoffDelay*10^<numberOfRetries>


nit: I'm more accustomed to using base 2 for the backoff (so just double the delay each time instead of 10x it), but do not feel super strongly about this.
Thought? Do we want to cap this after say 10 times or something. This can be a todo later guided by experience.

I got this from the k8s go client. Now I see it has changed (or I was not looking at the right place): https://github.com/kubernetes/client-go/blob/master/util/workqueue/default_rate_limiters.go#L65

Fixing...

vaikas · 2019-10-09T23:32:43Z

+```go
+
+// DeliverStatus contains the Status of an object supporting delivery options.
+type DeliverStatus struct {


We have DeliverySpec is DeliverStatus a typo? I'd expect them to be consistent.

vaikas · 2019-10-09T23:40:02Z

+)
+
+// DeliverStatus contains the Status of an object supporting delivery options.
+type DeliverStatus struct {


same here, I think this should be DeliveryStatus for consistency?

good catch!

sayanh · 2019-10-18T12:57:22Z

+
+Note that multiple copies of the same event can be sent to the error sink due to multiple subscription failures.
+
+Brokers might decide to change the event type before reposting the failed event into the broker. This could be done by having a special error sink specific to broker.


Did you mean brokers will aggregate the events(in case of multiple copies) based on id? Why brokers will change the event type? If done, the subscribers will be impacted. Can you give an example where it makes sense to change the event type?

alexander-alvarez · 2019-10-28T20:08:35Z

@lionelvillard any way to get insight into the design document for those of us outside of the sharing circle?

This capability is a deal breaker for us to trial and later adopt knative for particular use cases

lionelvillard · 2019-10-28T20:34:06Z

Not sure how to do that. @vaikas-google ?

vaikas · 2019-10-29T16:50:51Z

easiest would to join the knative-users@ google group, will give access to the documents in general.
https://github.com/knative/community

vaikas · 2019-10-29T16:51:36Z

/lgtm
/approve

knative-prow-robot · 2019-10-29T16:51:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas-google]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vaikas · 2019-10-29T17:36:14Z

/lgtm

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 23, 2019

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 23, 2019

knative-prow-robot requested review from Harwayne and matzew September 23, 2019 17:31

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2019

lionelvillard mentioned this pull request Sep 23, 2019

Add ErrorChannel to Channelable #1838

Closed

lionelvillard force-pushed the dlc branch from 773c39f to c201377 Compare September 23, 2019 17:45

mattmoor-sockpuppet reviewed Sep 23, 2019

View reviewed changes

lionelvillard force-pushed the dlc branch from 7618aeb to 1783406 Compare September 25, 2019 13:14

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2019

mattmoor-sockpuppet reviewed Sep 25, 2019

View reviewed changes

lionelvillard force-pushed the dlc branch 2 times, most recently from 1747896 to 3e9bb46 Compare September 25, 2019 15:20

mattmoor-sockpuppet reviewed Sep 25, 2019

View reviewed changes

n3wscott reviewed Sep 25, 2019

View reviewed changes

aslom mentioned this pull request Sep 25, 2019

Dead-Letter Queue or other Error Handling #1573

Closed

lionelvillard changed the title ~~Extend Channelable with DLC support~~ Initial error handling support Sep 25, 2019

lionelvillard force-pushed the dlc branch from 5e27514 to 7129ffa Compare September 25, 2019 18:02

matzew reviewed Sep 26, 2019

View reviewed changes

knative-prow-robot assigned vaikas Oct 7, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2019

vaikas reviewed Oct 9, 2019

View reviewed changes

matzew mentioned this pull request Oct 10, 2019

Adding matzew to approvers #2033

Merged

sayanh reviewed Oct 18, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2019

lionelvillard added 10 commits October 29, 2019 13:09

Add ErrorChannel to Channelable

9ca88b9

sync with latest design

5220149

move delivery spec to own file

67b1e32

add delivery design as markdown

bb45658

fix typos

3d1da46

use event everywhere. use deadLetterSink and deadLetterChannel

d325d5c

update deliveryspec

796938d

deadLetterSink can be one per subscriber

ab4a9aa

fixes after review

e154312

rebase

a5d784c

lionelvillard force-pushed the dlc branch from 1df9031 to a5d784c Compare October 29, 2019 17:12

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

knative-prow-robot merged commit cb91d30 into knative:master Oct 29, 2019

lionelvillard deleted the dlc branch August 28, 2020 15:38

evankanderson mentioned this pull request May 9, 2021

Channel go definitions: ¯\_(ツ)_/¯ #5364

Closed


		### Dead-Letter Channel

		Channel implementations might leverage the existing native error handling support they provide, usually a dead letter channel, to forward failed messages to the error sink. In that case, the error sink might be realized by creating a subscription on the error channel.


		### Delivery Specification

		The goal of this delivery specification is to formally define the vocabulary related to capabilities defined above (error sink, dead-letter queues and retry) to provide consistency across all Knative event sources, channels and brokers.


		### Exposing underlying DLC

		Channels supporting dead letter queue should advertise it in their status.


		Note that multiple copies of the same event can be sent to the error sink due to multiple subscription failures.

		Brokers might decide to change the event type before reposting the failed event into the broker. This could be done by having a special error sink specific to broker.

Conversation

lionelvillard commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

lionelvillard commented Sep 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionelvillard commented Oct 7, 2019

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sayanh Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-alvarez commented Oct 28, 2019

Uh oh!

lionelvillard commented Oct 28, 2019

Uh oh!

vaikas commented Oct 29, 2019

Uh oh!

vaikas commented Oct 29, 2019

Uh oh!

knative-prow-robot commented Oct 29, 2019

Uh oh!

vaikas commented Oct 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lionelvillard commented Sep 23, 2019 •

edited

Loading

sayanh Oct 18, 2019 •

edited

Loading