WIP: enable channels to dispatch events to a subscriber subset by lionelvillard · Pull Request #1436 · knative/eventing

lionelvillard · 2019-06-19T21:18:45Z

Proposed Changes

Add routable field to ChannelableSpec
Add FanOut routable support, used by InMemoryChannel

Release Note

channels now support dispatching events to a subset of subscriptions

This PR implement the first part of the design document, and only for InMemoryChannel. There is no Router CRDs (this should be done as part of a separate PR)

I added some examples (only one for now)

knative-prow-robot · 2019-06-19T21:19:00Z

Hi @lionelvillard. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2019-06-19T21:19:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @n3wscott 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

deissnerk · 2019-06-28T23:53:18Z

+// ComputeRoutes sends the message to the router.
+func (d *MessageDispatcher) ComputeRoutes(message *Message, router string, defaults DispatchDefaults) ([]string, error) {
+	routerURL := d.resolveURL(router, defaults.Namespace)
+	req, err := http.NewRequest(http.MethodPost, routerURL.String(), bytes.NewReader(message.Payload))


A CoudEvent router will only need the attributes of the CloudEvent context. Sending the complete message to the router will not be necessary in all cases.

It's necessary for content-based routing. Are you suggesting the router should advertise what attributes it needs (could initially vs coarse-grained, context vs content vs both)?

I realize that there is also content-based routing, but the Broker concept so far completely relies on CloudEvents. After all, this is exactly what CloudEvents are designed for. To make it short, yes. 😄
I know that in this stage we should not optimize too much. I only brought it up, because routing based on the context attributes is a major use case.

evankanderson · 2019-07-01T22:53:18Z

/assign @grantr @vaikas-google

I think we may have wanted to keep Channel/Subscription as simple mappings on the underlying pubsub primitives, but I'll defer to Grant and Ville on this.

lionelvillard · 2019-07-01T23:12:40Z

maybe this does not have to be exposed in the CRD, and instead it should be treated as an optimization for advanced filtering.

evankanderson

I heard from @grantr there is a second proposal around routing floating around, and I'd love to compare it. 😁

I'm concerned with both the addition of complexity to all Channel implementations due to the Router contract, as well as the actual shape of the Routing API, which seems to bleed control plane and data plane concerns together more than I'd like.

evankanderson · 2019-07-01T22:45:03Z

+
+	// Arguments defines the arguments to pass to the Router
+	// +optional
+	Arguments *runtime.RawExtension `json:"arguments,omitempty"`


I think in the past we've had issues with "bag of JSON" or "bag of string-string" in terms of tooling usability and underspecifying the actual behavior. You might want to look at #930 for some of the discussion on filtering language and behavior.

I actually removed arguments in the latest proposal but not in the code.

evankanderson · 2019-07-01T22:48:05Z

 	Subscribable *eventingduck.Subscribable `json:"subscribable,omitempty"`
+
+	// Channel conforms to Duck type Routable.
+	Routable *eventingduck.Routable `json:"routable,omitempty"`


What happens if both subscribable and routable is set on the same object? Are they "inclusive and" or "xor"?

If they are "and", why not simply add Arguments to Subscription for this behavior? (Again, I think this may be too generic to be well-supported across implementations, but having two different types for interacting with Channel seems even more ambiguous.)

The current behavior is routable takes precedence.

evankanderson · 2019-07-01T22:50:14Z

+	if config.Router != nil && config.Subscriptions != nil {
+		handler.subscribers = make(map[string]int)
+		for i, sub := range config.Subscriptions {
+			// TODO: can we not deprecate ref?


IIRC, I think "ref" can get confusing when objects are deleted and re-created quickly. Using a ID prevents this sort of confusion.

evankanderson · 2019-07-02T00:25:32Z

+			}
+		}
+
+		for i := subsCount - len(routes); i >= 0; i-- {


IMO, this might be simpler to have routes be a set, and then iterate over subscribers and compare the name with the set. If no match, you can send back a nil. If there is a match, you can pop from the set and return the result of the request.

If you want to log any names that don't match Subscriptions as errors, you can then log them directly, and the accounting is a bit simpler to track, I think.

ideally we want to have another method to handle the router case so that we don't have to iterate over all subscriptions. I mean this could also be done in dispatch, but that'd cleaner.

evankanderson · 2019-07-02T00:27:59Z

+	}
+
+	var routes []string
+	err = json.Unmarshal(payload, &routes)


I'm uncomfortable with the implied contract here:

CloudEvent -> [ subscription_names ]

In particular, it feels like there's some spooky action-at-a-distance in that the subscription names are based on shared knowledge of apiserver state (which may be laggy), rather than information in the request itself.

I'd much prefer a protocol where the choices were included in the request payload, so that the function could be implemented without needing reference to a third system.

There is no apiserver state involved here. The subscription names are user-specified and the router does not manipulate any apiserver objects.

However there is the issue that the router can become out-of-sync with the channel subscribers, eg when manually adding Subscription. One solution is to provide two channel modes, pubsub vs routing. When a channel is in routing mode, only Router objects can manipulate the channel.

matzew · 2019-09-23T14:30:56Z

@lionelvillard is this still a thing?
Also note that @nachocano has a larger PR to refector the event_receiver/dispatcher

lionelvillard · 2019-09-23T14:58:25Z

I meant to close this. Might be part of the work on performance improvement.

* Revert "[SRVKS-790] Patch subresource to unblock webhooks on 4.9 (knative#1361) (knative#1365)" This reverts commit cef96eb. * upgrade to latest dependencies (knative#5707) bumping knative.dev/pkg dd0db4b...953af01: > 953af01 [release-0.24] allow unknown metadata fields (# 2255) > 03e7ca5 Drop redundant pointers and decoders (# 2260) Signed-off-by: Knative Automation <automation@knative.team> Co-authored-by: knative-automation <automation@knative.team>

enable channels to dispatch events to a subscriber subset

01d63b7

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2019

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

knative-prow-robot requested review from Harwayne and evankanderson June 19, 2019 21:18

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 19, 2019

deissnerk reviewed Jun 28, 2019

View reviewed changes

knative-prow-robot assigned grantr and vaikas Jul 1, 2019

evankanderson reviewed Jul 2, 2019

View reviewed changes

lionelvillard mentioned this pull request Jul 24, 2019

Broker internal design improvements #1555

Closed

lionelvillard closed this Sep 23, 2019

lionelvillard deleted the routing branch January 20, 2020 14:58

Conversation

lionelvillard commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Jun 19, 2019

Uh oh!

knative-prow-robot commented Jun 19, 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!

evankanderson commented Jul 1, 2019

Uh oh!

lionelvillard commented Jul 1, 2019

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

matzew commented Sep 23, 2019

Uh oh!

lionelvillard commented Sep 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lionelvillard commented Jun 19, 2019 •

edited

Loading