Proposal: remove broker ingress channel

[yolocs]

Problem
Broker currently creates two channels: trigger and ingress. The ingress channel is used to stage events replied by triggers. However, it seems like it's a meaningless additional hop that introduced latency and reliability issue. I think a better approach is to directly send replies to the ingress service.

One might argument the ingress channel can help with reliability. Since a reply is a http response, there is no indicator of whether the response is successful or not. A lost reply means a lost event that cannot be recovered. And because (in most cases) the ingress channel provides durability, it can help preventing losing replied events.

This is not true because if the ingress channel is down, we will anyway lose replied events. This is essentially the same as directly replying to the ingress service except that the ingress channel itself is yet another potential point of failure.

How should we do if the ingress service is down? It probably worths its own issue but here is my rough idea.

The current flow is:

(channel outbound) -[req]-> (filter service) -[req]-> (consumer) -[resp with event]-> (filter) -[resp with event]-> (channel outbound) -[ack message]-> (the real channel e.g. kafka)
                                                                                                                                       |
                                                                                                                                       -[req with event]-> (replyURL)

The channel outbound can first try send the replied event to the replyURL, then decide whether to ack or unack the message. In this way, if it fails to deliver the replied event, the flow will be retried again.

Persona:
Dev

Exit Criteria

1. Ingress channel is removed from broker and replyURL is replaced with the ingress service endpoint.
2. (Optional for this issue) Adjust channel outbound logic to try deliver replies first.

Time Estimate (optional):
1 week.

[yolocs]

/area performance

[yolocs]

/assign @grantr

[mikehelmick]

related #1555

[yolocs]

/assign @yolocs

[grantr]

/unassign

[aslom]

@yolocs could you show what is current architecture - ingress channel how it is used in particualr?
Also in the flow what is channel outbound and why is it used twice?

(channel outbound) -[req]-> (filter service) -[req]-> (consumer) -[resp with event]-> (filter) -[resp with event]-> (channel outbound) -[ack message]-> (the real channel e.g. kafka)

[aslom]

We could discuss it in more detail Channels WG? Add to agenda https://docs.google.com/document/d/1uxlulaAf2m_yZUqCIeI-inul2gsqP69PElnZdO0FHUo/edit#heading=h.pd8ef9j5o38k

[yolocs]

@yolocs could you show what is current architecture - ingress channel how it is used in particualr?
Also in the flow what is channel outbound and why is it used twice?

"Channel outbound" is the channel component which pushes events to subscribers. For IMC, it's the imc-dispatcher. The flow above has [req] and [resp]. "Channel outbound" makes a [req] and in the end it will receive a [resp] - that's why it appeared twice.

I'll try to create a nicer flow chart to explain the broker internals.

[lionelvillard]

I drew one a long time ago. Not sure if it's accurate

[yolocs]

Thanks, @lionelvillard! Nice chart and I think it's still accurate.

What I'm proposing is to have (5) directly link back to the "broker ingress" rather than going through the "broker ingress channel" (called topic in this chart).

[yolocs]

Found in the comment:

eventing/pkg/reconciler/broker/broker.go

Lines 145 to 149 in ec187bd

	// 5. Ingress Channel is created to get events from Triggers back into this Broker via the
	// Ingress Deployment.
	// - Ideally this wouldn't exist and we would point the Trigger's reply directly to the K8s
	// Service. However, Subscriptions only allow us to send replies to Channels, so we need
	// this as an intermediary.

Since we now have Destination instead of Channel in the Reply. It seems there is no more reason to have the ingress channel.

[matzew]

+1 I guess :-)

/cc @vaikas @Abd4llA