add support for Subsciption.replyTo property by markfisher · Pull Request #325 · knative/eventing

markfisher · 2018-08-06T16:16:26Z

Fixes #216

Proposed Changes

Subscription now accepts an optional "replyTo" property
Bus implementations pass the Subscription's replyTo property when delegating to the common dispatcher
the dispatcher will forward a non-empty response body as a new request to the destination specified by the replyTo if present
headers will be propagated, including a correlation ID header if provided in the request

markfisher · 2018-08-06T16:16:55Z

/assign @evankanderson
/assign @scothis

markfisher · 2018-08-06T16:43:33Z

/test pull-knative-eventing-integration-tests

n3wscott · 2018-08-06T17:04:32Z

 					subscription.Name, subscription.Spec.Channel, subscription.Spec.Subscriber)
 				message := fromKafkaMessage(msg)
-				err := d.messageDispatcher.DispatchMessage(subscription.Spec.Subscriber, subscription.Namespace, message)
+				err := d.messageDispatcher.DispatchMessage(subscription.Spec.Subscriber, subscription.Spec.ReplyTo, subscription.Namespace, message)


These feels like we should wrap the message and replyTo in a struct and then DispatchMessage is (subscriber, event)?

Should we consider whether subscriber and namespace also belong in the struct? (would be a single "envelope" message parameter if so).

WDYT @scothis? I addressed all of the other comments (so far) besides this one.

The message and destination feel fundamental to me. I'd be in favor of something like:

func (d *MessageDispatcher) DispatchMessage(message *Message, destination string, opts *DispatchOpts) error

I went with a DispatchDefaults struct. RFAL @n3wscott @scothis. Thanks!

n3wscott · 2018-08-06T17:05:18Z

+	if err != nil {
+		return fmt.Errorf("Unable to complete request %v", err)
+	}
+	if replyTo != "" && res != nil {


wrap this in executeReply?

after refactoring so that executeRequest returns a Message (based on the suggestion by @scothis below), this will be reduced to a single call to executeRequest for the reply Message

scothis

I really like the direction, a few nits inline

scothis · 2018-08-06T16:52:46Z

+	return nil
+}
+
+func (d *MessageDispatcher) executeRequest(destination string, defaultNamespace string, message *Message) (*http.Response, error) {


executeRequest should return a Message rather than an http response so that the http transport remains encapsulated.

scothis · 2018-08-06T16:57:32Z

+// The destination and replyTo are DNS names. For names with a single label,
+// the default namespace is used to expand it into a fully qualified name
+// within the cluster.
+func (d *MessageDispatcher) DispatchMessage(destination string, replyTo string, defaultNamespace string, message *Message) error {


perhaps rename replyTo to defaultReplyTo to indicate in the future there may be other sources of metadata to configure replies.

scothis · 2018-08-06T16:59:19Z

+			return fmt.Errorf("Unable to read response %v", err)
+		}
+		replyMessage := Message{headers, payload}
+		d.executeRequest(replyTo, defaultNamespace, &replyMessage)


should an error sending the reply be returned instead of suppressed?

scothis · 2018-08-06T17:06:26Z


 var forwardPrefixes = []string{
+	// knative
+	"x-knative-",


maybe just knative- since the x- prefix is generally discouraged

n3wscott · 2018-08-08T22:18:38Z

 	Subscriber string `json:"subscriber"`

+	// Target service DNS name for replies returned by the subscriber.
+	ReplyTo string `json:"replyTo"`


I think replyTo is optional, so the json annotation should be json:"replyTo,omitempty"

indeed, good catch!

n3wscott · 2018-08-08T22:26:57Z

/lgtm

scothis

/lgtm

scothis · 2018-08-08T22:53:12Z

 				Payload: pubsubMessage.Data,
 			}
-			err := b.messageDispatcher.DispatchMessage(subscriber, namespace, message)
+			defaults := buses.DispatchDefaults{Namespace: sub.Namespace, ReplyTo: sub.Spec.ReplyTo}


nit: put each property on a dedicated line

scothis · 2018-08-08T22:56:16Z

+		headers := d.fromHTTPHeaders(res.Header)
+		// TODO: add configurable whitelisting of propagated headers/prefixes (configmap?)
+		correlationID := message.Headers[correlationIDHeaderName]
+		if correlationID != "" {


this can be combined with the previous line:

if correlationID, ok := message.Headers[correlationIDHeaderName]; ok { headers[correlationIDHeaderName] = correlationID }

thanks, changed

fixes knative#216

markfisher · 2018-08-09T14:03:56Z

@n3wscott @scothis I addressed the latest round of review comments; please re-lgtm if you're ok with it

n3wscott · 2018-08-09T14:17:11Z

/lgtm

/test pull-knative-eventing-integration-tests

scothis · 2018-08-09T14:18:10Z

/lgtm
@evankanderson PTAL

In the future, we should be smarter about when we create a reply message. Generally, I'd only expect a reply to be created for a 2xx status codes that also has a non-empty body.

markfisher · 2018-08-13T22:53:09Z

please have a look when you can @evankanderson

thanks!

evankanderson · 2018-08-13T23:41:01Z

Sorry about that, this fell off my radar.

I'm okay experimenting with this approach, but I have a few concerns:

How will this fit with an eventual Composer/Function Workflows type environment. In particular, I think @bbrowning or @csantanapr may have mentioned that OpenWhisk uses something more like a "mailbox/cubbyhole" for tracking message responses to particular events.
It seems like this doesn't do much for request/response type abstractions; if you deliver an event to a channel, there's no easy way to get the response back. (It will go to N places specified by N subscriptions.)
I suspect we want a URL rather than a DNS name.
It seems like this introduces a correlation ID without documenting or motivating it.

At this point, I'm willing to approve this PR as long as we all agree that we aren't too attached to this implementation yet, and that's it's more of a "spike" than a designed and accepted pattern.

We should probably have a discussion over the next few weeks about how to enable this sort of experimentation to proceed without confusing the overall repo between code that we're trying to raise to production quality and experimental forays that aren't part of our roadmap at this point.

Maybe annotations and/or PoC branches would be a good way to manage this?

evankanderson · 2018-08-13T23:43:27Z

/approve

but I'd really like to see us focus on raising the coverage numbers for pkg/controller/channel, pkg/controller/bus, and pkg/buses before adding more features.

knative-prow-robot · 2018-08-13T23:43:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, markfisher, n3wscott

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~~ [evankanderson]

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

markfisher · 2018-08-14T00:05:54Z

@evankanderson I completely agree this should be treated as just an initial spike from which we can experiment... for example I have this rough PoC for request/reply correlation (your point 2, also makes use of the correlation-id you mention in point 4) and I'll start working on something similar for the "activation claim check" pattern (what you referred to as "mailbox/cubbyhole" in point 1). The key is that this initial layer of "chaining" can be a common foundation for both approaches (and likely others). Thanks for the approval. Looking forward to the next steps!

* add support for Subsciption.replyTo property fixes knative#216 * addressing review comments

@evankanderson

@evankanderson

From [ROLES.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/ROLES.md#approver): > Reviewer of the codebase for at least 3 months or 50% of project lifetime, whichever is shorter - [First Issue](#80). Opened 6/11 - [First PR](#66). Opened 5/31 - [First Review](#79 (review)) 6/11 > Primary reviewer for at least 10 substantial PRs to the codebase - #422 (review) - #414 (review) - #325 (review) - #225 (review) - #189 (review) - #168 (review) - #165 (review) - #99 (review) - #79 (review) - #111 (review) > Reviewed or merged at least 30 PRs to the codebase - [Reviewed 23 PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+reviewed-by%3Ascothis) - [Authored 34 merged PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+author%3Ascothis+is%3Amerged) - [Authored 5 open PRs](https://github.com/knative/eventing/pulls/scothis) > Nominated by an area lead From [WORKING_GROUPS.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/WORKING-GROUPS.md#events) /assign @vaikas-google > With no objections from other leads 🤞 /cc @evankanderson @grantr @inlined @mattmoor

@evankanderson

Co-authored-by: pierDipi <pierDipi@users.noreply.github.com>

knative-prow-robot requested review from inlined and mattmoor August 6, 2018 16:16

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 6, 2018

knative-prow-robot assigned evankanderson and scothis Aug 6, 2018

markfisher force-pushed the chaining-issue216 branch from c7be6f8 to 0d139e6 Compare August 6, 2018 16:19

markfisher changed the title ~~wip~~ add support for Subsciption.replyTo property Aug 6, 2018

n3wscott reviewed Aug 6, 2018

View reviewed changes

scothis reviewed Aug 6, 2018

View reviewed changes

markfisher force-pushed the chaining-issue216 branch 2 times, most recently from 0fb1607 to bbe2fb6 Compare August 8, 2018 13:50

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2018

n3wscott approved these changes Aug 8, 2018

View reviewed changes

knative-prow-robot assigned n3wscott Aug 8, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2018

scothis reviewed Aug 8, 2018

View reviewed changes

add support for Subsciption.replyTo property

9951f11

fixes knative#216

markfisher force-pushed the chaining-issue216 branch from bbe2fb6 to ee11b04 Compare August 9, 2018 13:54

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2018

markfisher force-pushed the chaining-issue216 branch from ee11b04 to c450495 Compare August 9, 2018 13:58

addressing review comments

a23834d

markfisher force-pushed the chaining-issue216 branch from c450495 to a23834d Compare August 9, 2018 14:01

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2018

jldec mentioned this pull request Aug 10, 2018

CLI support for subscription replyTo projectriff/riff#672

Closed

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2018

knative-prow-robot merged commit 0195820 into knative:master Aug 14, 2018

n3wscott pushed a commit to n3wscott/eventing that referenced this pull request Aug 14, 2018

add support for Subsciption.replyTo property (knative#325)

df138cd

* add support for Subsciption.replyTo property fixes knative#216 * addressing review comments

scothis mentioned this pull request Sep 11, 2018

Add scothis as an approver #426

Merged

n3wscott mentioned this pull request Sep 25, 2018

adding n3wscott to approvers. #465

Merged

matzew pushed a commit to matzew/eventing that referenced this pull request Oct 12, 2023

Run make generate-release (knative#325)

819fe5e

Co-authored-by: pierDipi <pierDipi@users.noreply.github.com>

Conversation

markfisher commented Aug 6, 2018

Proposed Changes

Uh oh!

markfisher commented Aug 6, 2018

Uh oh!

markfisher commented Aug 6, 2018

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!

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

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Aug 8, 2018

Uh oh!

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

markfisher commented Aug 9, 2018

Uh oh!

n3wscott commented Aug 9, 2018

Uh oh!

scothis commented Aug 9, 2018

Uh oh!

markfisher commented Aug 13, 2018

Uh oh!

evankanderson commented Aug 13, 2018

Uh oh!

evankanderson commented Aug 13, 2018

Uh oh!

knative-prow-robot commented Aug 13, 2018

Uh oh!

markfisher commented Aug 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants