Skip to content

Add ErrorChannel to Channelable#1838

Closed
lionelvillard wants to merge 1 commit into
knative:masterfrom
lionelvillard:duckerror
Closed

Add ErrorChannel to Channelable#1838
lionelvillard wants to merge 1 commit into
knative:masterfrom
lionelvillard:duckerror

Conversation

@lionelvillard
Copy link
Copy Markdown
Contributor

@lionelvillard lionelvillard commented Sep 9, 2019

Helps #1573 and #1493

Proposed Changes

  • Add ErrorChannel field in Channelable.

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

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 9, 2019
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 9, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Sep 9, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lionelvillard
To complete the pull request process, please assign vaikas-google
You can assign the PR to them by writing /assign @vaikas-google in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

matzew
matzew reviewed Sep 10, 2019
View reviewed changes
Comment thread pkg/apis/duck/v1alpha1/channelable_types.go
// subscribers. One message is sent to this channel per failing subscriber.
//
// +optional
ErrorChannel *apisv1alpha1.Destination `json:"errorChannel,omitempty"`
Copy link
Copy Markdown
Member

@matzew matzew Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each channel object, I can do a "custom" error channel? 🤔

Copy link
Copy Markdown
Contributor Author

@lionelvillard lionelvillard Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for security reason. Potentially each channel is associated with an ACL and by default the error channel should have the same ACL. In practice, most channels send failed messages to a globally shared error channel but we need to allow for restricted channels.

Does that make sense to you?

Copy link
Copy Markdown
Member

@matzew matzew Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that makes sense.

@Harwayne
Copy link
Copy Markdown
Contributor

Harwayne commented Sep 10, 2019

/hold

Holding while we review the design.

@knative-prow-robot 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 10, 2019
@lionelvillard
Copy link
Copy Markdown
Contributor Author

lionelvillard commented Sep 23, 2019

closing in favor of #1949

@lionelvillard lionelvillard deleted the duckerror branch January 20, 2020 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matzew matzew matzew left review comments

@n3wscott n3wscott Awaiting requested review from n3wscott

@vaikas vaikas Awaiting requested review from vaikas

Assignees

No one assigned

Labels

cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lionelvillard @knative-prow-robot @Harwayne @matzew @googlebot