Implement SSE events transport. Add new event types. Update README #79

x-cray · 2015-10-28T22:01:51Z

Kind of fixes #19
Please review the PR, I'm a bit new to Go

eicca · 2015-10-28T22:19:04Z

As current code doesn't fully pass golint, would it make sense to write the new code in a proper way, or in a consistent way? Writing the golintable code would make it easier to refactor the old code in the future, however it will look inconsistent. What do you think, what's better?

eicca · 2015-10-28T22:21:54Z

README.md

+		for {
+			event := <-update
+			glog.Infof("EVENT: %s", event)
+		}


Since you're touching the examples anyway, what do you think about removing else statements from them?

Ok, I'll also modify actual example code file.

eicca · 2015-10-28T22:29:05Z

I wonder if it's possible to have some tests for this addition. Do you have any thoughts about this?

x-cray · 2015-10-28T22:29:47Z

@eicca As I mentioned earlier, I'm new to Go =) I've checked golint output and it seems that there are many places it complains about. I'll fix points which are related to this PR, but IMO others should be fixed is separate PRs to maintain consistent intent of this specific PR.

x-cray · 2015-10-28T22:50:09Z

@eicca In terms of tests I think that we might mock SSE server like in this example https://github.com/donovanhide/eventsource/blob/master/example_event_test.go and send predefined set of events expecting to receive them from this channel https://github.com/gambol99/go-marathon/blob/master/subscription.go#L47

gambol99 · 2015-10-30T14:57:43Z

client.go

-	}
+	// Try to connect to Marathon until succeed or
+	// the whole custer is down
+	for {


👍 definitely easier to read when using the for loop ...

gambol99 · 2015-10-30T15:04:41Z

👍 LGTM

x-cray · 2015-10-30T15:08:12Z

@gambol99 I'd like to add some tests to it.

eicca · 2015-10-30T20:57:38Z

subscription.go

+	// Prevent multiple SSE subscriptions
+	if r.subscribedToSSE {
+		return nil
+	}


It's still could be affected by a race condition. Would it be better to try to use https://golang.org/pkg/sync/#Once? Or that would be totally users fault to achieve the race condition here and the boolean field is enough? :)

@eicca Good point. But I see another issue here: registerSSESubscription() is only called from within RegisterSubscription() which in turn is only called from AddEventsListener() which is already synchronised. The issue is that RegisterSubscription itself is being exported while it is not defined in Marathon interface. I think that we could just rename RegisterSubscription to registerSubscription and that's it. What do you think?

x-cray · 2015-11-02T09:24:55Z

@gambol99 @eicca Question: should Marathon.UnSubscribe() be exported to interface? It is being called automatically upon event listener removal in Marathon.RemoveEventsListener(). Would it be better is we don't export it? What's the purpose of this method if we call it from outside while having some event subscriptions active?
I'm not even sure that this line is correct since UnSubscribe() should have been already called earlier.

eicca · 2015-11-02T21:38:46Z

@x-cray I'm sorry for the lack of activity in this PR, unfortunately I don't have enough knowledge of the marathon events. I can try to dig into it a bit later once I have more time.

gambol99 · 2015-11-02T22:41:48Z

@x-cray good spot! and no under this context it shouldn't really be exported, neither should the RegisterSubscription() from the looks for it, as the {Add,Remove}EventsListener() methods can handled it all. Off the top of my head though, it might be useful to keep the method but change its function slightly

from
func (r *marathonClient) UnSubscribe() error 
to 
func (r *marathonClient) UnSubscribe(url string) error

And then in https://github.com/gambol99/go-marathon/blob/master/subscription.go#L66 where it checks for any more listeners we could change to

if len(r.listeners) <= 0 {
    r.UnSubscriber(r.SubscriptionURL())
}

Internally the consumer should use the Add/RemoveEventslisten(channel) to handle events, but they can also have a generic method to delete subscriptions(url) if they like, i.e. list all and clean up subscriptions unrelated to the Add/Register methods etc ... what you reckon? ..

x-cray · 2015-11-03T08:51:46Z

@gambol99 In my opinion your abstraction over Marathon events API with {Add/Remove}EventsListener() is really great and it makes UnSubscribe() unnecessary. However, your suggestion to add url param adds some sense to it, but only in situation when we need to unsubscribe some third-party subscriber. Could be. Will you make this change in upstream?

gambol99 · 2015-11-03T10:12:48Z

@x-cray

However, your suggestion to add url param adds some sense to it, but only in situation when we
need to unsubscribe some third-party subscriber

agreed ..

Will you make this change in upstream?

If this PR doesn't need to change it i'm happy to raise the change and PR myself? ...

x-cray · 2015-11-03T18:13:30Z

If this PR doesn't need to change it i'm happy to raise the change and PR myself? ...

I think that it's not related to PR. I've only changed this line by adding check for correct EventsTransport to unsubscribe only when using callback event transport.

For the rest, I'm happy with all my changes now =) I've added functional tests for event stream. The most notable change in unit testing runtime infrastructure is the way how test HTTP server is started now — for every test case a new instance is being started and stopped afterwards. That was required for event stream testing and in general it's always good practice to prepare test environment from scratch for every distinct test and then dispose it.

gambol99 · 2015-11-04T21:48:50Z

testing_utils.go

+	"github.com/donovanhide/eventsource"
+	"github.com/golang/glog"
+	yaml "gopkg.in/yaml.v2"
+	"sync"


'sync' should probably be placed in the above section

Sure, missed that one

gambol99 · 2015-11-04T21:55:08Z

Aside from the one tiny comment ... LGTM ... awesome PR and cheers for fixing up alot of the linting issues

Move GetEvent() to events.go as it as nothing to do in subscription. Introduce separate type for EventsTransport. Update README, add SSE example, make other examples to be in line with golint rules. Don't export registerSubscription() method. Add functional tests. Fix functional tests runtime infrastructure.

Implement SSE events transport. Add new event types. Update README

eicca reviewed Oct 28, 2015
View reviewed changes

This was referenced Oct 29, 2015

Golint conformance #80

Closed

generic logger #66

Closed

gambol99 reviewed Oct 30, 2015
View reviewed changes

gambol99 added the enhancement label Oct 30, 2015

eicca reviewed Oct 30, 2015
View reviewed changes

gambol99 reviewed Nov 4, 2015
View reviewed changes

gambol99 added a commit that referenced this pull request Nov 5, 2015

Merge pull request #79 from x-cray/sse-transport

40f4d42

Implement SSE events transport. Add new event types. Update README

gambol99 merged commit 40f4d42 into gambol99:master Nov 5, 2015

gambol99 mentioned this pull request Nov 5, 2015

unsubscribe url parameter #86

Merged

x-cray deleted the sse-transport branch November 6, 2015 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SSE events transport. Add new event types. Update README #79

Implement SSE events transport. Add new event types. Update README #79

x-cray commented Oct 28, 2015

eicca commented Oct 28, 2015

eicca Oct 28, 2015

x-cray Oct 28, 2015

eicca commented Oct 28, 2015

x-cray commented Oct 28, 2015

x-cray commented Oct 28, 2015

gambol99 Oct 30, 2015

gambol99 commented Oct 30, 2015

x-cray commented Oct 30, 2015

eicca Oct 30, 2015

x-cray Oct 30, 2015

x-cray commented Nov 2, 2015

eicca commented Nov 2, 2015

gambol99 commented Nov 2, 2015

x-cray commented Nov 3, 2015

gambol99 commented Nov 3, 2015

x-cray commented Nov 3, 2015

gambol99 Nov 4, 2015

x-cray Nov 5, 2015

gambol99 commented Nov 4, 2015

Implement SSE events transport. Add new event types. Update README #79

Implement SSE events transport. Add new event types. Update README #79

Conversation

x-cray commented Oct 28, 2015

eicca commented Oct 28, 2015

eicca Oct 28, 2015

Choose a reason for hiding this comment

x-cray Oct 28, 2015

Choose a reason for hiding this comment

eicca commented Oct 28, 2015

x-cray commented Oct 28, 2015

x-cray commented Oct 28, 2015

gambol99 Oct 30, 2015

Choose a reason for hiding this comment

gambol99 commented Oct 30, 2015

x-cray commented Oct 30, 2015

eicca Oct 30, 2015

Choose a reason for hiding this comment

x-cray Oct 30, 2015

Choose a reason for hiding this comment

x-cray commented Nov 2, 2015

eicca commented Nov 2, 2015

gambol99 commented Nov 2, 2015

x-cray commented Nov 3, 2015

gambol99 commented Nov 3, 2015

x-cray commented Nov 3, 2015

gambol99 Nov 4, 2015

Choose a reason for hiding this comment

x-cray Nov 5, 2015

Choose a reason for hiding this comment

gambol99 commented Nov 4, 2015