What's the role of BrokerSpec.Delivery outside mtbroker implementations?

[slinkydeveloper]

My understanding of BrokerSpec.Delivery documentation (https://github.com/knative/eventing/blob/master/pkg/apis/eventing/v1/broker_types.go#L77) is that it leaks the details of the mtbroker implementation:

// Delivery is the delivery specification for Events within the Broker mesh.

In fact this is used inside the broker controller to configure the subscription to internal channels https://github.com/knative/eventing/blob/master/pkg/reconciler/mtbroker/resources/subscription.go#L63. This should be decoupled from the spec itself, for example providing this delivery configuration through a config map and address it with BrokerSpec.Config (which is exactly the role of that field).

What this means outside the mtbroker implementation? How should we interpret it in other broker implementations?

E.g. this delivery spec, within the broker mesh, doesn't make sense in Kafka, because delivery spec (retries in particular) doesn't map to kafka semantics, where we have the at least once guaranteed by the protocol itself through coordination between sender and receiver (acks).

Additional Context

In case of eventing-kafka-broker we wrongly interpreted it and we use it as a "global default" of delivery for each trigger. If a trigger has a delivery spec, then use the trigger specific, otherwise use the global one, if any. I guess this probably has more sense as usage for this field.

How did you interpreted this field in gcp broker @grantr?

Proposals

• Change the meaning to "defaulter" for trigger delivery spec
• Remove it

[matzew]

E.g. this delivery spec, within the broker mesh, doesn't make sense in Kafka, because delivery spec (retries in particular) doesn't map to kafka semantics, where we have the at least once guaranteed by the protocol itself through coordination between sender and receiver (acks).

seems like imp detail that was exposed, to early ?

well, or for channel based brokers.. (unlike w/ Rabbit / Kafka broker)... but than the Broker type has no more direct ref w/ the Channels...

so that field feels "wrong"

[slinkydeveloper]

Looping @pierDipi and @vaikas too in the discussion, which are working on the other broker impls

[lionelvillard]

In case of eventing-kafka-broker we wrongly interpreted it and we use it as a "global default" of delivery for each trigger. If a trigger has a delivery spec, then use the trigger specific, otherwise use the global one, if any. I guess this probably has more sense as usage for this field.

that's the original intent.

[slinkydeveloper]

that's the original intent.

If that's the case, then the doc is wrong/unclear and our usage in mtbroker is wrong too

[pierDipi]

If a trigger has a delivery spec, then use the trigger specific, otherwise use the global one, if any.

Change the meaning to "defaulter" for trigger delivery spec

A trigger has no delivery spec.

If that's the case, then the doc is wrong/unclear and our usage in mtbroker is wrong too

This is related to this issue: #4057 (We should re-open it)

[grantr]

In GCP broker we interpret broker's delivery spec as a global default for all triggers attached to the broker (google/knative-gcp#1536). The backend is capable of per-trigger delivery specs, but we don't have an API that can set them.

We also have a configmap that sets a default delivery spec for all brokers (google/knative-gcp#1566).

We currently rely on PubSub to implement retry and dead letter policy. Some translation is required to get the generic delivery spec into a form supported by PubSub. This is explained in more detail in https://github.com/google/knative-gcp/blob/master/docs/spec/delivery.md.

• PubSub only supports exponential backoff so we will reject the Linear backoff setting (Properly implement linear backoff in Broker delivery spec google/knative-gcp#1832).
• We translate BackoffDelay and Backoff Policy to PubSub's minimum and maximum backoff values.
• PubSub only supports topics as dead letter queues, so we define a custom URI scheme to specify topic names. Arbitrary dead letter sinks are not supported. It's possible to support them but it hasn't been a priority.

I haven't thought about it too carefully, but maybe it would be reasonable to define a DeliveryPolicy CRD with a webhook that applies its policy to triggers matching a selector, annotating the trigger with the specifics of the policy applied. This would allow for a default policy specification and leave the door open for alternate versions of that CRD that are implementation-specific.

@tommyreddad implemented all this so he's more expert than me on the details.

[vaikas]

My $.02 here is, do we have a concrete use case for having a per trigger delivery spec that's different from a Broker specific. I know there's a lot of things we can do, but before complicating things by having various places to have this configured, want to make sure that we have a concrete use case (aka, somebody is trying to do something that they can't). I haven't heard anybody asking for it, which is not to say nobody's asked for it, but just asking.

[slinkydeveloper]

In GCP broker we interpret broker's delivery spec as a global default for all triggers attached to the broker (google/knative-gcp#1536). The backend is capable of per-trigger delivery specs, but we don't have an API that can set them.

So I guess you misinterpreted like we did in eventing-kafka-broker 😄 . What should we do with this field then?

[slinkydeveloper]

I haven't thought about it too carefully, but maybe it would be reasonable to define a DeliveryPolicy CRD with a webhook that applies its policy to triggers matching a selector, annotating the trigger with the specifics of the policy applied. This would allow for a default policy specification and leave the door open for alternate versions of that CRD that are implementation-specific.

TBH this sounds like a next step to the simpler solution of supporting Delivery in trigger, which should be pretty much straightforward to implement for all broker implementations i know out there. Although this is not the point of this issue, so maybe can we open a new one for that? I'm interested in this too.

[lberk]

@slinkydeveloper have you gotten answers to your questions above? Should we open a new issue to track the different issue as you described?

[slinkydeveloper]

I think we still didn't agreed on this issue, let me go through it at today eventing delivery wg

[vaikas]

@slinkydeveloper can this be closed now?

[slinkydeveloper]

No, need to implement the change to the mtbroker

[slinkydeveloper]

/assign