Fixes #246 - moved event stream processing to engine api #315

jhorwit2 · 2016-07-13T22:43:46Z

Signed-off-by: Joshua Horwitz [email protected]

Move the events logic to the engine-api while keeping backwards compatibility with the original Events method. Porting this logic to the engine-api makes it easier to write libraries utilizing the events code. I was having to use both docker core & engine-api while writing a library. Docker core has some dependencies that aren't needed by external libraries.

vdemeester · 2016-07-14T08:54:06Z

This is also related #246

jhorwit2 · 2016-07-14T12:57:12Z

@vdemeester I noticed that after starting to work on this PR. I noticed your lib takes into account types; do you think I should incorporate that into this PR? I initially was trying to keep it close to how core docker has it.

The only thing I was thinking that should be different if we did add type/action handlers is to use a struct for keys over strings to make sure we don't have a collisions in the future. Another option is to just add a wildcard action instead of adding types. Then you can use the event options to set {"type": {"container": true}}, then eh.Handle(types.HandleAnyEvent, cb)

stevvooe · 2016-07-18T21:56:36Z

Consider some of the suggestions in #89 (comment) for an API. I'm not sure if a callback-based API is the correct approach if you have to listen for errors on a channel anyways. This pattern is employed here.

jhorwit2 · 2016-07-19T13:55:27Z

client/events.go

+			case event := <-eventsq:
+				processer := handler.Get(event.Action)
+				if processer != nil {
+					processer(event)


Keep going back and forth here on whether or not to do this in another goroutine. Thoughts? I like not because then you keep to sequential processing which can be an important thing with events.

That won't provide much. Serialization is cheap.

jhorwit2 · 2016-07-19T13:55:55Z

@stevvooe updated from your feedback 👍

stevvooe · 2016-07-19T20:08:55Z

client/events.go

+// handlers for processing. It's up to the caller to stop the processing by canceling
+// the context. The error channel will be closed once the end of the input stream has been
+// reached. If an error is received all processing will be stopped.
+func (cli *Client) EventsWithHandler(ctx context.Context, options types.EventsOptions, handler eventtypes.EventHandler) <-chan error {


I'd rather see something like this:

Events(ctx context.Context, options types.EventsOptions) (<-chan eventtypes.Message, <-chan error)

Then, we can have a wrapper built with a handler on top of this.

The issue with the handler approach is that it requires too much boilerplate to get something useful. A handler system can be built on the above, but it is hard to readapt the handler system to channel based system, if that is required instead.

You put Events as the name. Can I replace the legacy method with this new one? I figured we needed backwards compatibility so I went with another method name.

Can I replace the legacy method with this new one? I figured we needed backwards compatibility so I went with another method name.

All the users of the engine-api vendor the dependency, so please feel free to change.

As it is, the method is very hard to use, so replacing the definition is fine. We'll stabilize these eventually, but we need to have an exported API worth stabilizing and we're not there yet.

jhorwit2 · 2016-07-21T16:25:33Z

@stevvooe i think this is what you had in mind 👍

stevvooe · 2016-07-21T21:44:26Z

@jhorwit2 Love the new Events signature, but the handler needs more design work.

Do you mind splitting the handler parts into a second PR?

stevvooe · 2016-07-21T21:44:49Z

Mostly, I'd like to get the Events signature merged... 👍 👍

jhorwit2 · 2016-07-21T21:48:59Z

Sounds good! Just pushed w/o the handler code @stevvooe.

stevvooe · 2016-07-21T21:55:10Z

@jhorwit2 Thanks for the awesome contribution!

LGTM

Presumably, wait until have 1.12 GA until we merge this but it is up to engine-api maintainers.

cc @aaronlehmann @vdemeester

vdemeester · 2016-07-22T07:45:25Z

@jhorwit2 Looks very good, thanks 😻
LGTM 🐸

But let's wait the 1.12 release before merging this 👼

vdemeester · 2016-08-09T14:34:24Z

@jhorwit2 would you like to make a PR in docker/docker with these changes to see how it behaves ? 👼

jhorwit2 · 2016-08-09T19:25:30Z

@vdemeester yea sure! ill cherry pick the commit in and open a pr with vendor changes.

jhorwit2 · 2016-08-16T13:21:12Z

client/events.go

+func (cli *Client) Events(ctx context.Context, options types.EventsOptions) (<-chan events.Message, <-chan error) {
+
+	messages := make(chan events.Message, 1)
+	errs := make(chan error, 1)


@stevvooe You mentioned in previous comment to make this a buffered channel in case the error wasn't consumed, but when you do this you force the consumer to check the event channel after an error has been received because both the event and error channel can have data.

messages shouldn't be buffered, only errq.

Derp... good catch.

stevvooe · 2016-08-26T23:56:58Z

LGTM

PTAL @tonistiigi @aaronlehmann

tamird · 2016-08-27T00:04:29Z

client/events_test.go

+				buffer := new(bytes.Buffer)
+
+				for _, e := range eventsCase.events {
+					b, _ := json.Marshal(e)


is it really worth it here to not check the error?

Also, it's probably more efficient to use a json.Encoder:

buffer := new(bytes.Buffer) enc := json.NewEncoder(buffer) for _, e := eventsCase.events { if err := enc.Encode(e); err != nil { // do something } }

just realized this is the test! still, would be good to check the error.

tonistiigi · 2016-08-27T00:11:17Z

Isn't it bit awkward to return 2 channels? Also when using select on these 2 channels it is easy to skip last messages because you could get the error first. Why not return a object instead with method for getting the events or error. Or channel returning union of these fields.

Signed-off-by: Joshua Horwitz <[email protected]>

jhorwit2 · 2016-08-27T00:14:13Z

@tonistiigi because the message channel isn't buffered that won't happen

tamird · 2016-08-27T00:14:46Z

Yeah, an object with a Next() method that returns (events.Message, error) might make more sense.

stevvooe · 2016-08-27T00:16:18Z

@tamird That would disable the ability to select on a channel while consuming. You could always build a wrapper that does the Next() pattern.

tamird · 2016-08-27T00:25:02Z

That's true.

aaronlehmann · 2016-08-27T00:29:41Z

client/events.go

+			select {
+			case <-ctx.Done():
+				return
+			default:


This pattern makes sense, but I would end this select here to reduce unnecessary indentation.

aaronlehmann · 2016-08-27T00:34:27Z

LGTM

lowenna · 2016-08-30T20:34:33Z

LGTM. Thanks!

jhorwit2 · 2016-09-08T15:17:31Z

So what's the plan with this since engine-api is now deprecated?

tonistiigi · 2016-09-08T16:22:16Z

@jhorwit2 Just combine it with moby/moby#25853 and copy this code to github.com/docker/docker/client. The use case of that package remains the same, we still expect other projects to import it if they want a docker client library.

vdemeester · 2016-09-08T17:24:09Z

anyway let's close this now as it is all in moby/moby#25853

GordonTheTurtle added the status/0-triage label Jul 13, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Jul 14, 2016

jhorwit2 force-pushed the jah/events-engine-api branch 2 times, most recently from 9b27ea3 to 1194e3f Compare July 19, 2016 13:52

jhorwit2 reviewed Jul 19, 2016
View reviewed changes

jhorwit2 force-pushed the jah/events-engine-api branch 3 times, most recently from b93f7e7 to e0b0080 Compare July 19, 2016 14:09

stevvooe reviewed Jul 19, 2016
View reviewed changes

jhorwit2 force-pushed the jah/events-engine-api branch 6 times, most recently from ffc2373 to bc698e0 Compare July 20, 2016 16:31

jhorwit2 force-pushed the jah/events-engine-api branch from bc698e0 to a8a4f69 Compare July 21, 2016 21:48

vdemeester added status/2-code-review status/wait-until-release-freeze and removed status/1-design-review labels Jul 22, 2016

vdemeester removed the status/wait-until-release-freeze label Jul 31, 2016

stevvooe mentioned this pull request Aug 1, 2016

client.ImagePull needs a JSON message type #89

Open

jhorwit2 reviewed Aug 16, 2016
View reviewed changes

jhorwit2 force-pushed the jah/events-engine-api branch 2 times, most recently from 9258520 to 50949ed Compare August 16, 2016 19:40

jhorwit2 mentioned this pull request Aug 18, 2016

Refactor to new engine-api events api moby/moby#25853

Merged

vdemeester mentioned this pull request Aug 26, 2016

Introduce watchContainers in service… docker/libcompose#290

Closed

tamird reviewed Aug 27, 2016
View reviewed changes

Moved event stream processing into engine api

863e816

Signed-off-by: Joshua Horwitz <[email protected]>

jhorwit2 force-pushed the jah/events-engine-api branch from 50949ed to 863e816 Compare August 27, 2016 00:11

aaronlehmann reviewed Aug 27, 2016
View reviewed changes

vdemeester closed this Sep 8, 2016

jayhding mentioned this pull request Oct 26, 2016

Complete logs command docker/libcompose#210

Closed

Fixes #246 - moved event stream processing to engine api #315

Fixes #246 - moved event stream processing to engine api #315

Conversation

jhorwit2 commented Jul 13, 2016

vdemeester commented Jul 14, 2016

jhorwit2 commented Jul 14, 2016 • edited Loading

stevvooe commented Jul 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhorwit2 commented Jul 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhorwit2 commented Jul 21, 2016

stevvooe commented Jul 21, 2016

stevvooe commented Jul 21, 2016

jhorwit2 commented Jul 21, 2016

stevvooe commented Jul 21, 2016

vdemeester commented Jul 22, 2016

vdemeester commented Aug 9, 2016

jhorwit2 commented Aug 9, 2016

jhorwit2 Aug 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Aug 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Aug 27, 2016

jhorwit2 commented Aug 27, 2016

tamird commented Aug 27, 2016

stevvooe commented Aug 27, 2016

tamird commented Aug 27, 2016

Choose a reason for hiding this comment

aaronlehmann commented Aug 27, 2016

lowenna commented Aug 30, 2016

jhorwit2 commented Sep 8, 2016

tonistiigi commented Sep 8, 2016 • edited Loading

vdemeester commented Sep 8, 2016

jhorwit2 commented Jul 14, 2016 •

edited

Loading

jhorwit2 Aug 16, 2016 •

edited

Loading

tonistiigi commented Sep 8, 2016 •

edited

Loading