Record the decision on Broker and Trigger #862

evankanderson · 2019-03-07T07:20:03Z

Fixes #818

Records a decision from the 2019-02-22 Decision Making -- UX Targets document.

Implicitly establishes a location in GitHub for recording the parameters of specific decisions from the eventing WG.

Proposed Changes

Documents plan-of-record for Broker and Trigger objects in eventing.knative.dev, targeted for 0.5.0

Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>

knative-prow-robot · 2019-03-07T07:20:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

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

evankanderson · 2019-03-07T07:20:15Z

/hold

To allow discussion while people may be on planes, different timezones, etc.

duglin · 2019-03-07T13:06:01Z

docs/decisions/broker-trigger.md

+
+Broker implements the "Addressable" duck type to allow Source custom resources
+to target the Broker. Broker DNS names should also be predictable (e.g.
+`default-broker` for the `default` Broker), to enable whitebox event producers


Does this implies that the "default" broker MUST always be present? e.g. if someone deletes it the system will recreate it?

There's a k8s watch set up on the namespaces and if the appropriate label is found there, then the Broker is created. So if a user manually deletes the Broker it will be recreated.

I don't read this as requiring the default Broker to be present. Rather, that when a whitebox event producer is given a Broker to send events to, they can fully construct the DNS name for that Broker, without needing to get the Broker object itself.

From earlier in the doc:

It is expected that most users will not need to explicitly create a Broker and simply enabling eventing in the namespace to create the default Broker will be sufficient.

So we think that most of the time it will be the default Broker, but nothing requires it to be. Nor does anything require the default Broker to exist.

Added a mention earlier about the mechanism for provisioning the default broker, which is (as Ville suggests) that the broker is created by reconciling the namespaces with the appropriate annotation.

duglin · 2019-03-07T13:16:28Z

docs/decisions/broker-trigger.md

+select the events. In the case of multiple Triggers with the same target whose
+filter conditions select an overlapping set of events, events will be delivered
+once for each Trigger which selects the event (i.e. Trigger selection criteria
+are independent, not aggregated in the Broker).


How is this uniqueness determined? Some discussions in CloudEvents about this one.

I don't understand the question. What 'uniqueness' is being determined?

I think what this section is trying to say is that conceptually, each event that enters the Broker will be tested against every Trigger independently. Thus if multiple Triggers match that incoming event, then that incoming event is sent to the Subscriber of all those Triggers. If some of those Triggers have the same Subscriber, then that Subscriber will be sent the incoming event multiple times (once for every Trigger that matches it and has that Subscriber).

Yes, what Adam said. I'll try to clear this up.

docs/decisions/broker-trigger.md

vaikas · 2019-03-07T13:20:57Z

docs/decisions/broker-trigger.md

+
+Broker implements the "Addressable" duck type to allow Source custom resources
+to target the Broker. Broker DNS names should also be predictable (e.g.
+`default-broker` for the `default` Broker), to enable whitebox event producers


There's a k8s watch set up on the namespaces and if the appropriate label is found there, then the Broker is created. So if a user manually deletes the Broker it will be recreated.

docs/decisions/broker-trigger.md

duglin · 2019-03-07T13:34:57Z

docs/decisions/broker-trigger.md

@@ -0,0 +1,137 @@
+# Broker and Trigger model (for 0.5.0)
+
+Decision date: 6 March 2019


Let's add a list of terms/components with a one-liner for each one - e.g.:

Event Producer - creates events based on occurrences. E.g. github

Event Consumer - destination for events from an Event Producer. E.g. the application processing an event. Typically, this is the final "sink" in the processing flow of an Event.

Event Source - a helper entity that can be used to connect an Event Producer to the Kn eventing system and a destination (sink) for those events (e.g. an Event Consumer or a Broker)

Event Source Service - a Knative Service that is the endpoint to which Event Producers send events. This is created by the Event Source

Sink - a generic term to mean the destination for an event being sent from a component.

Broker - an entity that can be a Sink. Subscriptions can be created for the Broker to register interest in Events that flow through the Broker. Brokers are typically used to provide loose coupling between the incoming events and Event Consumers - e.g. it may buffer/queue events.

Trigger/Subscription - a registered interest in events that flow through a Broker. This will typically include a Sink (where to send the Events) and an optional filter to control which Events are sent. It may buffer/queue events.

Not sure about both the Broker and Trigger buffering things.... did we talk about that? I noticed below it talks about the Trigger doing the buffering.

I think including this would help. In particular I like is that the terms have a concrete example associated with them.

I don't think 'Event Source Service' is needed. Or if it is included, it should not mandate that it is a Knative Service (e.g. it could be a K8s Deployment).

Event Producer should also specify that the event produced does not need to be a CloudEvent.

Conceptually, each Trigger has an independent buffer. The implementation of where the buffer lives in the data plane is a bit mushier, as I expect that the Broker and Trigger will collapse into the same data-plane objects in many cases.

I've added some updated definitions (for example, the "Event Source Service" is called a "Receive Adapter", and might be a Deployment or StatefulSet instead of a Knative Service, depending on the Event Source implementation).

I left out the following two definitions, because they are defined later in the document in more detail:

Broker: an event dispatch object which acts as an event Sink. Triggers can
be created on the Broker to register interest in Events that are delivered to
the Broker. Brokers are used to provide loose coupling and content-based
routing between incoming events and Event Consumers - e.g. it may
buffer/queue events.

Trigger a registered interest in events that flow through a Broker. This
will typically include a Sink (where to send the Events) and an optional
filter to control which Events are sent. It may buffer/queue events.

duglin · 2019-03-07T13:37:23Z

docs/decisions/broker-trigger.md

+Broker. For the MVP, Triggers may only target Addressable objects in the same
+namespace.
+
+#### Buffering


Did we decide that buffering is done in the trigger instead of the broker? I can see an argument for either.

What is the difference between buffering in one vs the other?

Conceptually, the buffering applies to each Trigger -- if you have the following objects:

Broker B
Services X and Y
Trigger T1 matching type=foo pointing to X created at t1
Trigger T2 matching all events pointing to Y created at t4

And the following sequence of events delivered to the Broker:
t0: type=foo id=123
t2: type=bar id=124
t3: type=foo id=125
t5: type=foo id=126
t6: type=bar id=127

Then X will receive 125 and 126, and Y will receive 126 and 127.

Harwayne

Overall looks good to me, although the YAMLs I have been working with look a bit different.

Harwayne · 2019-03-07T22:16:26Z

docs/decisions/broker-trigger.md

+select the events. In the case of multiple Triggers with the same target whose
+filter conditions select an overlapping set of events, events will be delivered
+once for each Trigger which selects the event (i.e. Trigger selection criteria
+are independent, not aggregated in the Broker).


I don't understand the question. What 'uniqueness' is being determined?

I think what this section is trying to say is that conceptually, each event that enters the Broker will be tested against every Trigger independently. Thus if multiple Triggers match that incoming event, then that incoming event is sent to the Subscriber of all those Triggers. If some of those Triggers have the same Subscriber, then that Subscriber will be sent the incoming event multiple times (once for every Trigger that matches it and has that Subscriber).

akashrv · 2019-03-11T21:24:35Z

docs/decisions/broker-trigger.md

+  - "filter" has exact type match
+  - "filter" has exact source match
+  - target is same-namespace
+- Registry requirements are not clear enough to include in MVP


Need clarification. Does this mean
a) Registry is not in scope for MVP?
OR
b) Registry is in scope for MVP but not yet specd out?

My understanding is that MVP scope is defined from a end customer perspective. - What is the minimal experience that is needed to get a meaningful feedback?
The scope is being discussed in the user stories document, the spec and detail requirements could be discussed in the UX proposal doc.

There was no clear consensus as to how the registry should be implemented or what functionality was needed; my assumption is that our MVP includes only functionality we can describe and implement.

I would assume that clear user stories which compellingly motivated the registry would also lend clarity to whether the Registry is in scope for MVP. Absent such evidence, I must conclude that it is not.

I understand that there was no clear consensus on registry design. However the bigger question is scope of MVP.
As per the user stories doc we have discussed, registry is seems to be in scope for MVP.
Given the current timelines we are targeting my suggestion will be to start with registry design and include an experimental version of MVP in 0.5 which will be enough for end users to play with, knowing that it is an experimental feature. This way we can have a complete experience shipped in 0.5, blog about it, and get feedback which will help us design registry incrementally.

Removed the phrase "MVP" in favor of the slightly wordier "this decision". Does that help?

docs/decisions/broker-trigger.md

evankanderson · 2019-03-12T17:00:12Z

Sorry, I've been out the last few days. I'll update this PR with the current comments this morning, but consider this a comment notice for closing discussion.

googlebot · 2019-03-12T17:57:20Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

evankanderson · 2019-03-12T18:07:49Z

Wow, Visual Studio screwed up that merge. Fixing.

googlebot · 2019-03-12T18:13:03Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

docs/decisions/broker-trigger.md

vaikas · 2019-03-12T18:23:31Z

docs/decisions/broker-trigger.md

+
+## Definitions
+
+- **Event Producer**: a system which creates events based on occurrences. E.g.


If an application (Event Consumer) produces an event in response to an event to process, do we need to mention it here? As in, only wondering if we should clarify that transformations are possible with the current system.

I think transformations will muddle this more than clarify; today, we can treat this as an Event Consumer and an Event Producer.

evankanderson · 2019-03-12T18:38:01Z

/retest

evankanderson · 2019-03-12T18:44:23Z

Related #814 #815

evankanderson · 2019-03-12T20:55:08Z

/retest

evankanderson · 2019-03-12T20:59:05Z

Note, this does not include resolving the naming document

katfang · 2019-03-13T01:06:21Z

docs/decisions/broker-trigger.md

+
+### Trigger
+
+A Trigger represents a registration of interest (request for delivery to an


The fact that there is a "to"-direction is important in the definition of a Trigger and deserves to be lifted out of the parenthetical.

How about "A Trigger represents a registration of interest in a filtered selection of events (in | routed through | accepted by | handled by) the Broker to be delivered to an Addressable object or URL."?

Thanks, done.

evankanderson · 2019-03-14T18:11:22Z

/hold cancel

This still needs an /lgtm. I'm going to ask @vaikas-google to do the honors.

vaikas · 2019-03-14T19:57:37Z

/lgtm

evankanderson · 2019-03-14T20:10:46Z

/retest

grantr · 2019-03-14T20:42:50Z

Looks like mdlint prefers expanded tabs:

I0314 20:11:25.353] docs/decisions/broker-trigger.md:67: MD010 Hard tabs
I0314 20:11:25.354] docs/decisions/broker-trigger.md:68: MD010 Hard tabs
I0314 20:11:25.354] docs/decisions/broker-trigger.md:69: MD010 Hard tabs
I0314 20:11:25.354] docs/decisions/broker-trigger.md:70: MD010 Hard tabs
I0314 20:11:25.354] docs/decisions/broker-trigger.md:74: MD010 Hard tabs
I0314 20:11:25.354] docs/decisions/broker-trigger.md:78: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:80: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:121: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:122: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:123: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:124: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:128: MD010 Hard tabs
I0314 20:11:25.355] docs/decisions/broker-trigger.md:129: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:130: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:131: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:132: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:136: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:137: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:138: MD010 Hard tabs
I0314 20:11:25.356] docs/decisions/broker-trigger.md:142: MD010 Hard tabs
I0314 20:11:25.357] docs/decisions/broker-trigger.md:143: MD010 Hard tabs

evankanderson · 2019-03-14T21:03:00Z

I fixed lint's little red wagon. #907.

/retest

duglin · 2019-03-15T00:56:42Z

docs/decisions/broker-trigger.md

+  Producer to the Knative eventing system by routing events to an Event
+  Consumer. E.g. the
+  [GitHub Knative Source](https://github.com/knative/eventing-sources/tree/master/pkg/reconciler/githubsource)
+- **Receive Adapter**: a data plane entity (Knative Service, Deployment, etc)


I agree with this definition because it implies to be its more like a proxy. It "performs the event routing". And in that scenario the consumer would expect things like the CE-source to be something related to the original event producer. However, I know this view is not universally shared. So, we need to decide... is a Receiver Adapter just a piece of middleware or does it become the Event Producer ? And if the answer is "it depends on the Event Source" then let's call that out so it's clear that event consumer can't assume one model for all Receive Adapters.

I think you're actually thinking of the type, not source context attribute?

Some guidance from CloudEvents would be helpful here; I think currently CloudEvents describes middleware (including routers) as "Event Consumers" and then "Event Producers". 😉

In all seriousness, I think this is something important to agree on, but I think it will also require (at least) a second decision process and document if Knative wants to make a recommendation for "Event Sources" as defined here. I think "Event Importer" would probably have been a better term. Fortunately, we haven't burned the "Importer" term yet...

Nope, I do mean "source" - assuming you're talking about this sentence:

And in that scenario the consumer would expect things like the CE-source to be something related to the original event producer.

See: cloudevents/spec#404 for my thoughts on this.

But, in general I view Knative's job w.r.t. event sources as utilities to help the user subscribe to event producers and to get those events delivered to their app. To the app, the fact that Kn is in the picture at all is irrelevant to the app doing its job of processing the event. And, in fact, asking the app to know about Kn makes it harder for them to move the code some place else. As I've mentioned in some slack chats, if the event producer already generated CloudEvents then these event adapters would be more like proxies. So, in that light, the fact that they need to do "some work" to create the CloudEvent isn't material to what's going on from an app's POV - its just necessary busywork to comply with CE. The adapter is not meant to really add any semantic value beyond compliance with CE - so therefore, making the CE appear like it came from the adapter instead of the real event producer/source goes against that view.

[0.17] using fqn image name

Initial definitions of Broker and Trigger based on decision docs

445e172

Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>

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

knative-prow-robot assigned grantr, Harwayne and vaikas Mar 7, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 7, 2019

knative-prow-robot requested review from matzew and vaikas March 7, 2019 07:20

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 7, 2019

duglin reviewed Mar 7, 2019

View reviewed changes

docs/decisions/broker-trigger.md Outdated Show resolved Hide resolved

vaikas reviewed Mar 7, 2019

View reviewed changes

duglin reviewed Mar 7, 2019

View reviewed changes

Harwayne reviewed Mar 7, 2019

View reviewed changes

akashrv reviewed Mar 11, 2019

View reviewed changes

docs/decisions/broker-trigger.md Show resolved Hide resolved

evankanderson added 2 commits March 12, 2019 10:07

Interim commit to proxy content home.

82d807b

Update broker/trigger decision doc with feedback.

ec41008

knative-prow-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2019

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 12, 2019

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Mar 12, 2019

Merge remote-tracking branch 'upstream/master' into pineapple-rifleman

6d8a4b8

googlebot removed the cla: no Indicates the PR's author has not signed the CLA. label Mar 12, 2019

knative-prow-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 12, 2019

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

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2019

vaikas reviewed Mar 12, 2019

View reviewed changes

Address @vaikas-google comments.

2a58173

katfang reviewed Mar 13, 2019

View reviewed changes

Harwayne mentioned this pull request Mar 13, 2019

Broker and Trigger OpenAPI schema #894

Closed

Update with @katfang wording

85bf85f

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

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

evankanderson mentioned this pull request Mar 14, 2019

Remove markdown lint presubmit check in favor of the sockpuppet. #907

Merged

knative-prow-robot merged commit 81f115f into knative:master Mar 14, 2019

duglin reviewed Mar 15, 2019

View reviewed changes

devguyio mentioned this pull request Jan 29, 2020

Decide on the future of the Broker and Trigger #2460

Closed

matzew added a commit to matzew/eventing that referenced this pull request Oct 6, 2020

Merge pull request knative#862 from matzew/fqn_image_second_try_0.17

ec19b0b

[0.17] using fqn image name

		@@ -0,0 +1,137 @@
		# Broker and Trigger model (for 0.5.0)

		Decision date: 6 March 2019


		## Definitions

		- Event Producer: a system which creates events based on occurrences. E.g.


		### Trigger

		A Trigger represents a registration of interest (request for delivery to an

Record the decision on Broker and Trigger #862

Record the decision on Broker and Trigger #862

Conversation

evankanderson commented Mar 7, 2019 • edited Loading

Proposed Changes

knative-prow-robot commented Mar 7, 2019

evankanderson commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harwayne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Mar 12, 2019

googlebot commented Mar 12, 2019

evankanderson commented Mar 12, 2019

googlebot commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Mar 12, 2019

evankanderson commented Mar 12, 2019

evankanderson commented Mar 12, 2019

evankanderson commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Mar 14, 2019

vaikas commented Mar 14, 2019

evankanderson commented Mar 14, 2019

grantr commented Mar 14, 2019

evankanderson commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Mar 7, 2019 •

edited

Loading

duglin Mar 7, 2019 •

edited

Loading