Start of event-delivery service by inlined · Pull Request #40 · knative/eventing

inlined · 2018-04-16T22:59:41Z

First pass at creating the skeleton for a delivery service. Since this needs to run at data-plane scale, I've spliced it off from the bind-controller pod. Eventually this will become more complex (e.g. when the backing store is Kafka or some other durable queue). I want to experiment with the in-memory queue for a while so we can derive our requirements before we choose what backing store to endorse.

Future PRs will make the action dynamic once Bind has been refactored. May also add a firehose "sendEvent" method, which would be a good fishbowl implementation of a public event provider SDK.

* Changes to "triggers": * Renamed the "triggers" pacakge to "sources" to avoid confusion. * EventTriggers (FKA Sources) now take EventTriggers instead of just params. * source.EventTrigger is different from v1alpha1.EventTrigger because the latter has embedded Raw types that would be hard to use. * Updated GitHub sample: * Renamed the EventSource to "github.com" rather than just "github" to better align with GCF. I think this revealed something about the control plane model that I want to discuss. * Updated pullrequest.yaml to match new format. * As part of this I made a minor change to update-deps to work on mac (gnu-sed still required).

google-prow-robot · 2018-04-16T22:59:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inlined

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

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

inlined · 2018-04-16T23:00:12Z

/assign @vaikas-google

Sorry this didn't break into smaller PRs well =/

inlined · 2018-04-18T16:36:49Z

Changing base to branch v0.1 so it is appropriate to /assign @eobrain while Ville is out of office.

inlined · 2018-04-18T16:42:45Z

/assign @eobrain

inlined · 2018-04-18T16:42:58Z

/unassign @vaikas-google

eobrain

Still not done with the review, but here is what I have so far. Mostly nits and comments.

eobrain · 2018-04-19T04:17:35Z

@@ -0,0 +1,139 @@
+/*
+Copyright 2017 The Kubernetes Authors.


s/2017/2018/

eobrain · 2018-04-19T04:19:57Z

@@ -0,0 +1,139 @@
+/*
+Copyright 2017 The Kubernetes Authors.


Is "The Kubernetes Authors" the correct copyright owner? Or should it be Google like in the other licence files.

Seems like I copied a bad copy from cmd/controller.go

Fixed. I seemed to have copied bad prose from cmd/controller/main.go

eobrain · 2018-04-19T04:45:56Z

+		// but if it does, propagate it back.
+		glog.Info("Staring event sender")
+		if err := sender.Run(senderThreads, stopCh); err != nil {
+			glog.Fatalf("Error running controller: %s", err.Error())


Nit. Suggest:
glog.Fatalf("Error running controller: %v", err)

Never "%v" an error. "%s" has an overload for the error interface to call err.Error() whereas "%v" will inspect the structure (and fails to follow pointers)

eobrain · 2018-04-19T16:59:07Z

+    embed = [":go_default_library"],
+    importpath = "github.com/elafros/eventing/cmd/delivery",
+    pure = "on",
+    visibility = ["//visibility:public"],


Unnecessary, given the default_visibility.

eobrain · 2018-04-19T19:35:44Z

+	defer cancel()
+	srv.Shutdown(ctx)
+
+	glog.Flush()


Or alternatively this could be defer glog.Flush() at the beginning of the function.

eobrain · 2018-04-19T20:41:59Z

+// InMemoryQueue implements the queue interface with a memory buffer.
+// Note: this isn't just a simple typedef for a queue because we will soon start
+// experimenting with other features, such as fetching an event separate from acking,
+// transactional ack + enqueue, cursors, etc.


Do you mean that these other features will only apply to InMemoryQueue and not to the Queue interface? Is that why InMemoryQueue is exported (rather than being inMemoryQueue, with NewInMemoryQueue returning the interface?)

They'd apply to the Queue interface; I want to use silly shims to help me decide what I need/want before I go shopping for software

eobrain · 2018-04-19T20:45:13Z

+// Queue implements basic features to allow asynchronous buffering of events.
+type Queue interface {
+	Push(event QueuedEvent) error
+	Pull(stopCh <-chan struct{}) (event QueuedEvent, ok bool)


Does stopCh really belong here? It seems like instead it might be better to be passed into the NewInMemoryQueue function (or equivalent for other implementation), and stored in the struct.

Honestly I don't fully understand how to use stopCh. Until working with K8S I've been told that a public interface with a channel is a strict anti-pattern in Golang. Most of the code I see in bind/controller.go uses the stopCh similarly to how a context.Context is used (which would dictate that it is not used in a constructor). Otoh, wait.Until with an infinite loop callback means there can never be a graceful shutdown of the Bind controller. I'll need to ask Ville for help understanding the right usage when he's back in the office.

eobrain · 2018-04-19T20:48:47Z

+
+// TODO(vaikas): Remove this once Bind's Action has been migrated
+// to be generic.
+const alwaysUseProcessor = "eventing.elafros.dev/EventLogger"


The name of this constant make it sound like it is a Boolean. Maybe rename it to something like onlySupportedProcessorForNow

eobrain · 2018-04-19T20:49:23Z

+	bindsLister listers.BindLister
+}
+
+// NewReceiver creates a new Reciever object to enqueue events.


nit: spelling

eobrain · 2018-04-19T20:52:20Z

+}
+
+// SendEvent enqueues an event data and Context for delivery to a particular action.
+func (r *Receiver) SendEvent(action queue.ActionType, data interface{}, context *event.Context) error {


Is there any reason this cannot be the exact same signature as Action.SendEvent?

It's still very much TBD in my mind which half of the queue (if not both) will look up and determine the action. This is totally agnostic of whether the webhooks are the same.

eobrain

Here's the rest of my review.

eobrain · 2018-04-19T22:42:20Z

+		w.WriteHeader(http.StatusNotFound)
+		return
+	}
+


Maybe for clarity, it seems like it might be worth doing

namespace, flowName := matches[1], matches[2]

and using those below.

eobrain · 2018-04-19T22:47:34Z

+)
+
+// If an event source knows the exact flow it is targeting it can bypass the work involved with
+// processing event triggers.


I could not understand how this comment relates to the regexp below. Where is this bypassing of work?

The expensive version is not yet implemented. I can change the comment for now.

eobrain · 2018-04-19T22:51:20Z

+	bind, err := r.bindsLister.Binds(string(matches[1])).Get(string(matches[2]))
+	if err != nil {
+		if errors.IsNotFound(err) {
+			fmt.Printf("Could not find Bind %s in namespace %s\n", matches[2], matches[1])


Is this left over from debugging? Suggest removing.

eobrain · 2018-04-20T00:45:47Z

+		return
+	}
+
+	w.WriteHeader(http.StatusOK)


How about putting something in the response body to make debugging easier. For example something like

w.Write([]byte("event sent"))

And if you do this you don't need the w.WriteHeader(http.StatusOK).

I'll remove the StatusOK. I don't want to start sending willy nilly response bodies, especially prose bodies.

eobrain · 2018-04-20T00:51:59Z

+	if err := r.SendEvent(actionFromBind(bind), data, context); err != nil {
+		glog.Error("Failed to enqueue event", err)
+		w.WriteHeader(http.StatusInternalServerError)
+		return


I understand why you don't want to expose the err caused by internal error, but maybe print something in the response body to make debugging easier. For example

w.Write([]byte(fmt.Sprintf("internal error attempting to send event to %s", bind.Spec.Action.RouteName)))

Might be easier in the next PR when we remove the shim actionFromBind

eobrain · 2018-04-20T01:23:36Z

+	go func() {
+		sender.Run(20, stopCh)
+		done = true
+		wait.Done()


At this point, all that has happened is that 20 goroutines have been spawned inside sender.Run, but they have not finished yet. Is that what you intended, or did you intend to wait until all the RunOnces are finished?

Fixed so that sender.Run blocks like the equivalent code in controller/bind

eobrain · 2018-04-20T01:27:32Z

+		done = true
+		wait.Done()
+	}()
+	if done {


Isn't there a race here? Is it not possible (if unlikely) the go-routine above could execute done = true before we get here?

Only if sender.Run can exit before closing the stopCh (per the above mentioned fix)

eobrain · 2018-04-20T01:34:19Z

+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/client-go/kubernetes:go_default_library",
+    ],
+)


Maybe add unit tests in this package.

I don't feel confident that I know enough of Elafros to write a good unit test except one that literally mocks & exercises my code. The Logging action isn't really testable unless glog has hooks.

Can look at the right way to add tests after Copenhagen

eobrain · 2018-04-20T01:35:36Z

+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/client-go/kubernetes:go_default_library",
+    ],
+)


Maybe add unit tests in this package.

eobrain · 2018-04-20T01:41:10Z

+	// Set up HTTP endpoints
+	glog.Infof("Set up metrics scrape path %s", metricsScrapePath)
+	glog.Infof("Set up debug queue path %s", debugQueuePath)
+	glog.Infof("Hosting sendEvent API at prefix %s", sendEventPrefix)


It seems these logging statements would be better split up an put before their respective mux.Handle statements. Otherwise keeping these up-to-date will be error-prone.

Agreed. These were for my own debugging and they stuck around. Will reformat.

eobrain

A few extra godoc suggestions.

eobrain · 2018-04-20T03:39:41Z

+limitations under the License.
+*/
+
+package main


Suggest adding package comment explaining what this command is.

Not sure this is really necessary, but done.

eobrain · 2018-04-20T03:49:31Z

+limitations under the License.
+*/
+
+package delivery


Suggest adding package comment for godoc, perhaps in separately file doc.go containing just

/* package delivery blah blah blah ... */ package delivery

eobrain · 2018-04-20T03:56:32Z

+limitations under the License.
+*/
+
+package action


Suggest adding package comment.

(see doc.go)

inlined

Done with a pass at incorporating feedback.

I think that the queue package may merit tests, especially in the near future. I'm on the fence about whether it's worth building now rather than doing more demo work since the interface is so trivial.

WRT the Action package, the logger is untestable and the Elafros implementation would only literally make sure the code I wrote is exercised, not that any of the input is meaningfully acted upon. This can probably get exercised better in an integration test once we have a better foundation to work with.

inlined · 2018-04-23T20:45:40Z

@@ -0,0 +1,139 @@
+/*
+Copyright 2017 The Kubernetes Authors.


Fixed. I seemed to have copied bad prose from cmd/controller/main.go

inlined · 2018-04-23T20:45:45Z

+limitations under the License.
+*/
+
+package main


Not sure this is really necessary, but done.

inlined · 2018-04-23T20:48:51Z

+	go informerFactory.Start(stopCh)
+
+	// Start sender:
+	go func() {


You're right. It should have been blocking like the bind controller. Have fixed.

inlined · 2018-04-23T20:50:05Z

+	// Set up HTTP endpoints
+	glog.Infof("Set up metrics scrape path %s", metricsScrapePath)
+	glog.Infof("Set up debug queue path %s", debugQueuePath)
+	glog.Infof("Hosting sendEvent API at prefix %s", sendEventPrefix)


Agreed. These were for my own debugging and they stuck around. Will reformat.

inlined · 2018-04-23T20:51:15Z

+	defer cancel()
+	srv.Shutdown(ctx)
+
+	glog.Flush()


inlined · 2018-04-23T21:21:28Z

+		t.Fatal("Failed to pull queued event")
+	}
+	if !reflect.DeepEqual(context, event.Context) {
+		t.Fatalf("Event context was not marshalled correctly; expected=%+v got=%+v", context, event.Context)


Cascading failures in a test almost never help debugging. The only good reason for t.Errorf in the past was table-driven tests, and now there's a better interface that properly encapsulates t.Fatalf.

inlined · 2018-04-23T21:22:54Z

+	}
+}
+
+// RunOnce processes a single event from the queue.


The OK bool idiom is pretty idiomatic. LMK if the comment is sufficient.

inlined · 2018-04-23T21:23:23Z

+}
+
+// Run runs Sender until stopCh is closed.
+func (s *Sender) Run(threadiness int, stopCh <-chan struct{}) error {


Trying to mimic the surrounding codebase. I don't feel comfortable changing until/unless Ville clarifies.

inlined · 2018-04-23T21:23:50Z

+	go func() {
+		sender.Run(20, stopCh)
+		done = true
+		wait.Done()


Fixed so that sender.Run blocks like the equivalent code in controller/bind

inlined · 2018-04-23T21:25:00Z

+		done = true
+		wait.Done()
+	}()
+	if done {


Only if sender.Run can exit before closing the stopCh (per the above mentioned fix)

eobrain · 2018-04-24T23:33:13Z

+	Action  ActionType     `json:"action"`
+	Data    interface{}    `json:"data"`
+	Context *event.Context `json:"context"`
+}


OK. Suggest TODO saying that.

eobrain · 2018-04-24T23:40:32Z

+		t.Fatal("Failed to pull queued event")
+	}
+	if !reflect.DeepEqual(context, event.Context) {
+		t.Fatalf("Event context was not marshalled correctly; expected=%+v got=%+v", context, event.Context)


Not sure I agree, but OK -- fine as is.

eobrain · 2018-04-24T23:53:02Z

+	}
+}
+
+// RunOnce processes a single event from the queue.


Well, I naively expected the Boolean would mean "SendEvent succeeded". But actually the Boolean is also true in two cases that look like error conditions. It might be worth explaining how this is used to implement retry.

eobrain

Still LGTM

eobrain · 2018-04-25T18:59:37Z

/lgtm

Accept v0.1 and v0.2 cloud events. Adding UTs.

Olm manifest and catalogsource

inlined added 2 commits April 16, 2018 15:57

First pass at delivery service

b7b8ff9

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 16, 2018

google-prow-robot assigned vaikas Apr 16, 2018

This was referenced Apr 17, 2018

Event sources should be decoupled from actions #41

Closed

Add support for dynamic actions #44

Closed

inlined changed the base branch from master to v0.1 April 18, 2018 16:36

google-prow-robot assigned eobrain Apr 18, 2018

google-prow-robot unassigned vaikas Apr 18, 2018

eobrain suggested changes Apr 19, 2018

View reviewed changes

eobrain suggested changes Apr 20, 2018

View reviewed changes

review feedback

806c479

inlined commented Apr 23, 2018

View reviewed changes

inlined added 2 commits April 23, 2018 14:32

Automatic changes by build tools

2d2690c

Whoops. Forgot doc.gos

d940e90

eobrain approved these changes Apr 24, 2018

View reviewed changes

PR feedback; change interface in Sender

4387a13

eobrain approved these changes Apr 25, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2018

google-prow-robot merged commit 397222e into knative:v0.1 Apr 25, 2018

akashrv pushed a commit to akashrv/eventing that referenced this pull request Mar 14, 2019

Merge pull request knative#40 from nachocano/cloud_events

0d169db

Accept v0.1 and v0.2 cloud events. Adding UTs.

matzew added a commit to matzew/eventing that referenced this pull request Apr 8, 2019

Merge pull request knative#40 from matzew/olm_data

1218993

Olm manifest and catalogsource

matzew added a commit to matzew/eventing that referenced this pull request Apr 11, 2019

Merge pull request knative#40 from matzew/olm_data

5fddb69

Olm manifest and catalogsource

Conversation

inlined commented Apr 16, 2018

Uh oh!

google-prow-robot commented Apr 16, 2018

Uh oh!

inlined commented Apr 16, 2018

Uh oh!

inlined commented Apr 18, 2018

Uh oh!

inlined commented Apr 18, 2018

Uh oh!

inlined commented Apr 18, 2018

Uh oh!

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

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!

eobrain Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

eobrain Apr 19, 2018 •

edited

Loading