adding one more idea to the filter by n3wscott · Pull Request #1176 · knative/eventing

n3wscott · 2019-05-07T22:30:49Z

Add functions as a base idea to triggers and how it could support CEL.

n3wscott · 2019-05-07T22:31:11Z

n3wscott · 2019-05-07T22:31:38Z

/approve cancel

knative-prow-robot · 2019-05-07T22:31:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: grantr

If they are not already assigned, you can assign the PR to them by writing /assign @grantr in a comment when ready.

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

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

grantr

Let's definitely add this to the doc, because it's an interesting thought experiment. Here are my thoughts:

My impression of the Broker/Trigger model is that its value comes from its opinions. The opinions baked into Broker/Trigger have benefits for users and contributors: users have their basic needs met immediately and are guided along one particular, well-supported path, and contributors scope down the set of problems to only the chosen ones. Those outside the scope of the opinions chosen may need to implement their own path, but that's always possible with Channel and Subscription, which were intentionally designed to have as few opinions as possible (though they still have some opinions).

The proposal to give Trigger a URI filter basically makes Trigger equivalent to a Subscription. Since the proposal also makes the Trigger object an internal detail, I think we can s/Trigger/Subscription/ here.

Now the proposal is basically two parts:

Implement Trigger filtering internally by forwarding events to a function.
Create CRDs (one of which is proposed here) that encapsulate the choice of filtering behavior so users don't need to deal with writing and deploying and hooking up their own filtering functions.

This is interesting and elegant, but I think it will make Trigger more difficult to implement because the Broker can't assume anything about how the Trigger will behave. The Broker must send every event to each Trigger function always, and that removes the ability to optimize the Broker's implementation, which is the principal value of Broker over Channel as described above.

I think (but could be wrong) that the only possible implementation of this is a single Channel with one Subscription per Trigger. If that's true, I think there's no reason to make Trigger work this way, because it's exactly what Channel and Subscription already provide.

I suppose this proposal could be extended to add a BrokerCEL that knows how TriggerCEL works (or assumes it knows) and can optimize for that, but then we're back to the current (with #1047) Broker and Trigger.

grantr · 2019-05-07T23:31:53Z

+Another option could be to continue to use the orignal subscription filter
+function to allow for advanced filters without directly depending on CEL.
+
+Allow for the base trigger to take `spec.filter.uri` or `spec.fitler.ref`. Ref


spec.filter.ref

grantr · 2019-05-07T23:32:07Z

+Allow for the base trigger to take `spec.filter.uri` or `spec.fitler.ref`. Ref
+would be _addressable_ and resolve to a uri.
+
+The data plane contact of the filer uri/ref would be the same as subscriptions.


contract and filter uri/ref

grantr · 2019-05-07T23:34:43Z

+_Addressable_ resource.
+
+The side effect is CEL becomes an add-on. If we use services, we can use
+servings.service revisions for versioning.


This allows versioning the filtering code, but it doesn't solve the problem of versioning the CEL expression format. How would TriggerCEL specify which version of the CEL filtering Service it wanted to use? I think this is the same problem #1047 has with CEL versioning (the solution given there was to bump the CRD version when the CEL format changes).

evankanderson

Trigger dispatch to a function feels expensive and possibly a better fit for a simple Pipeline (#1067), rather than a built-in Trigger primitive.

In particular, I'm concerned how adding this option might interact with other optimizations like batching, authorization, and protocol negotiation.

Co-Authored-By: n3wscott <32305648+n3wscott@users.noreply.github.com>

googlebot · 2019-05-08T15:00:27Z

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.

googlebot · 2019-05-08T15:00:33Z

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.

Harwayne · 2019-05-09T16:40:46Z

+```yaml
+kind: TriggerCEL
+spec:
+  filter: >


Here spec.filter is a string, above it is an object. Can it be both?

This is the TriggerCEL spec. The above Trigger spec is a different definition, IIUC.

Ahh, thank you for pointing that out.

Harwayne · 2019-05-09T18:50:01Z

+```
+
+Then, we make a TriggerCEL that is exactly the same as the above proposal with
+the chance in resource name. 


chance -> change?

Harwayne · 2019-05-09T18:50:14Z

+```yaml
+kind: TriggerCEL
+spec:
+  filter: >


Ahh, thank you for pointing that out.

grantr · 2019-08-29T20:06:28Z

@n3wscott I'm closing this one. I've heard this from others in the meantime that they like this approach, so I'm sure it will come up when when we revisit advanced filtering. 😺

adding one more idea to the filter

38b699b

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

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 7, 2019

knative-prow-robot requested review from matzew and vaikas May 7, 2019 22:31

knative-prow-robot assigned grantr May 7, 2019

knative-prow-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2019

grantr reviewed May 7, 2019

View reviewed changes

evankanderson reviewed May 8, 2019

View reviewed changes

Comment thread docs/broker/filtering.md Outdated

Update docs/broker/filtering.md

ed37669

Co-Authored-By: n3wscott <32305648+n3wscott@users.noreply.github.com>

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 May 8, 2019

Harwayne reviewed May 9, 2019

View reviewed changes

grantr closed this Aug 29, 2019

Conversation

n3wscott commented May 7, 2019

Uh oh!

n3wscott commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n3wscott commented May 7, 2019

Uh oh!

knative-prow-robot commented May 7, 2019

Uh oh!

grantr 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!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

googlebot commented May 8, 2019

Uh oh!

googlebot commented May 8, 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!

grantr commented Aug 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

n3wscott commented May 7, 2019 •

edited

Loading