re #149 support event streaming #151

bsideup · 2015-07-25T15:30:17Z

I'm happy to contribute this small, but very helpful improvement - event stream listener.

Starting from Marathon 0.9.0 it provides /v2/events endpoint, which is implementation of Server-sent Events. It's much better compared to event callbacks since there is no need to register Bamboo as callback on Marathon.

Implementation is backward compatible with 0.7.0 because this mode is disabled by default. It could be enabled in config like that:

{
  "Marathon": {
    "Endpoint": "...",
    "UseEventStream": true
  }
}

NB: if "UseEventStream" is enabled then callbacks are not auto-registered.

When Bamboo will drop support for Marathon < 0.9.0 then this property could be true by default.

Implementation was tested with Marathon 0.9.0, covered cases:

Listening for events
Re-connection when Marathon is not responding
Bad data from Marathon
Many events at a time

lclarkmichalek · 2015-07-26T08:41:16Z

main/bamboo/bamboo.go

+
+				resp, err := client.Do(req)
+				if err != nil {
+					continue


Needs to be logged

How to handle stopped Marathon? Should I use exponential backoff (like this one: https://github.com/cenkalti/backoff) ?

I'm not sure that much complexity is needed, something as simple as giving every gorouting having a time.Ticker(time.Second), and then waiting for it before creating a new connection would be enough to ensure that Bamboo isn't spamming Marathon too hard, while still allowing the odd disconnection without error.

Maybe just log error and sleep for 1 second then? Usually backoff is used in such sutiations, but if you fill it complex then simple delay here should be fine

Ticker is redundant in case of just closed connection because closed connection doesn't mean to be a disaster, it could mean just "reconnect to me"

Let me describe whole scheme:

try to open connection

if failed, wait 1 sec and go to 1)

if not, read data from connection

if failed, log and go to 1 without delay

if not, read event and go to 3)

Which means that:
If failure happened while reading event on open connection, then whole cycle will start from 1)
If failure happened while connection opening then each attempt will happen not more than once per second.

So:
It's ok to wait 1 second if it's impossible to open connection to marathon.
If error happened during "read" phase, then we should retry connection immediately because it could be just dropped connection, but Marathon is still able to serve requests. If it's not, then it will loop on 1)-2).

Eh, it's roughly the same in the end.

No :) EOF (as well as other read errors) will not happen if your server is down because connection will not be open :)

https://en.wikipedia.org/wiki/Byzantine_fault_tolerance Treating particular errors specially isn't reliable. There are many things that can go wrong other than a server just being up or down.

meh...... ok, I will add Ticker. Still not sure that it's required here :)

lclarkmichalek · 2015-07-26T08:42:58Z

Looks good for the most part, I've added a couple of comments. On a broader note, I'd love to see some of this broken out into smaller functions; a function to parse a single message and return an event would be great, as it'd allow for very easy testing. At the moment, this is a rather had function to test.

bsideup · 2015-07-26T20:33:08Z

@bluepeppers I've made some changes, including:

Reuse code from EventSubscriptionAPI for parsing and dispatching events
Use Ticker for retrying
Log every error

Currently listenToEventStream contains only SSE handling logic. Should I split it more?

lclarkmichalek · 2015-07-26T21:42:41Z

I like this. I'd like to split it up more, and break the whole thing out behind an interface, but this is a move in the right direction. Seeing as this is opt in, I'd be happy merging it now, but will probably wait till Monday, and confer with @activars about the best way to test this mode with some decent traffic.

bsideup · 2015-07-27T08:42:07Z

@bluepeppers ok, lets wait what @activars will say :)

j1n6 · 2015-07-27T09:01:29Z

main/bamboo/bamboo.go

-	registerMarathonEvent(conf)
+	if conf.Marathon.UseEventStream {
+		// Listen events stream from Marathon
+		listenToEventStream(conf, eventSubAPI)


minor consistency issue. Do you think i's better to align with the callback model function name? e.g. renaming listenToEventSteam to registerMarathonEventStream

yes, you're right. Fixing...

Ah, stop :) listenTo<...> was used to be consistent with listenToZookeeper. registerMarathonEvent will register Bamboo in Marathon, but listenToEventStream will listen stream and will not register anything. So I will go with listenToEventStream if you don't mind

You are right. Maybe this is more accurate?
listenToMarathonEventStream

ok, no objections :) Will rename it now

Thank you :)

j1n6 · 2015-07-27T09:07:42Z

Looks good 👍
We will test this branch on our dev cluster.

j1n6 · 2015-07-27T09:09:07Z

don't worry about the abstraction too much, we will refactor some common features in next release.

drewrobb · 2015-07-31T21:56:05Z

docker build with this was broken for me-- use of the http.Client timeout requires go 1.4, the dockerfile uses go 1.2

drewrobb · 2015-08-11T02:24:56Z

You also may want to wait until the fix for mesosphere/marathon#1926 is included in a marathon release (probably 0.11). Otherwise there could in some cases be a delay for bamboo to receive events.

lclarkmichalek · 2015-09-08T14:43:22Z

@bsideup hey, sorry we haven't done anything on this. I've finally automated some bamboo integration testing stuff, and so I'll give this a shot on the staging cluster in the next week.

bsideup · 2015-09-08T16:42:19Z

Hi @bluepeppers ! Sounds great :) Ping me if anything will not work for you :)

lclarkmichalek · 2015-09-08T16:46:41Z

One minor thing; we have a username and password for marathon configurable: https://github.com/QubitProducts/bamboo/blob/master/configuration/marathon.go#L10-L15. The event stream request should probably support HTTP basic auth also

bsideup · 2015-09-14T12:22:44Z

@bluepeppers it was introduced after this PR :) But I can support it if you want

bsideup · 2015-09-14T12:26:18Z

@drewrobb good catch. Looks like ubuntu base image comes with Go 1.2 by default. Any idea how to fix it?

…nEventStream'

bsideup · 2015-09-14T12:34:19Z

@bluepeppers rebased on top of last master

bsideup · 2015-09-14T12:45:37Z

@bluepeppers implemented basic auth

lclarkmichalek · 2015-09-14T12:52:00Z

The go vet message at https://travis-ci.org/QubitProducts/bamboo/builds/80229954 was very useful. Without that change, the goroutine was listening to the same (last) marathon always, which causes the problem @drewrobb linked with marathon to be much more of an issue. I was having problems getting event streaming to work with this patch, but I think that go vet may have found the problem.

bsideup · 2015-09-14T12:55:54Z

@bluepeppers yes, already fixed :)

bsideup · 2015-10-15T11:35:48Z

@bluepeppers any news?

drewrobb · 2015-10-16T01:59:27Z

+1 to getting this in, I have been using it for a while now successfully

lavcraft · 2015-10-26T09:32:55Z

+1

j1n6 · 2015-10-27T16:59:24Z

Appologize we haven't got chance to test this out because there are a lot of other tasks we need to work on internally.
If anyone has time to test it out & sign-off on the status (with 3 thumbs up), we can speed up the release process.

We will deploy this on an internal test cluster to test it out in November.

bsideup · 2015-10-27T17:03:00Z

Since this is an optional mode I think it's pretty safe to merge, I know at
least 2 production deployments running my fork so it's battle tested:)
On Tue, 27 Oct 2015 at 6:59 PM Jing Dong notifications@github.com wrote:

Appologize we haven't got chance to test this out because there are a lot
of other tasks we need to work on internally.
If anyone has time to test it out & sign-off on the status (with 3 thumbs
up), we can speed up the release process.

We will deploy this on an internal test cluster to test it out in
November.

—
Reply to this email directly or view it on GitHub
#151 (comment).

lclarkmichalek · 2015-11-12T17:25:54Z

I'll merge this today (absent any issues). Sorry it took so long

bsideup · 2015-11-12T19:46:20Z

Awesome, thanks!
On Thu, 12 Nov 2015 at 8:58 PM Laurie Clark-Michalek <
notifications@github.com> wrote:

Merged #151 #151.

—
Reply to this email directly or view it on GitHub
#151 (comment).

lavcraft · 2015-11-13T11:25:59Z

Yeah, thanks guys!

bsideup · 2015-11-13T12:22:16Z

@bluepeppers was it released btw?

lclarkmichalek · 2015-11-13T12:50:00Z

No. The next release will probably be 0.3, with this and #180 being the features I want to test the most. I'm running master + #180 in dev atm, though there's still some extra work to be done around the UI for #180.

bsideup · 2015-11-13T12:50:42Z

@bluepeppers ok, keep me in touch then :) Thanks!

bsideup mentioned this pull request Jul 25, 2015

Please support event streaming #149

Closed

lclarkmichalek reviewed Jul 26, 2015
View reviewed changes

j1n6 reviewed Jul 27, 2015
View reviewed changes

Sergei Egorov added 5 commits September 14, 2015 15:30

re QubitProducts#149 support event streaming

2f17c2d

re QubitProducts#149 defer response closing and log close event

9ef0fd3

re QubitProducts#149 use Ticker for event stream connection restoration

ddfe126

re QubitProducts#149 reuse code from EventSubscriptionAPI

6ba64c6

re QubitProducts#149 rename 'listenToEventStream' to 'listenToMaratho…

af5a9df

…nEventStream'

bsideup force-pushed the events_stream branch from 766d294 to af5a9df Compare September 14, 2015 12:33

bsideup added 2 commits September 14, 2015 15:40

fix "range variable marathon captured by func literal"

1b10087

support Basic auth for event stream

7f0a0f8

drewrobb mentioned this pull request Oct 27, 2015

consult zookeeper for marathon endpoints #130

Open

lclarkmichalek merged commit 7f0a0f8 into QubitProducts:master Nov 12, 2015

re #149 support event streaming #151

re #149 support event streaming #151

Conversation

bsideup commented Jul 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lclarkmichalek commented Jul 26, 2015

bsideup commented Jul 26, 2015

lclarkmichalek commented Jul 26, 2015

bsideup commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j1n6 commented Jul 27, 2015

j1n6 commented Jul 27, 2015

drewrobb commented Jul 31, 2015

drewrobb commented Aug 11, 2015

lclarkmichalek commented Sep 8, 2015

bsideup commented Sep 8, 2015

lclarkmichalek commented Sep 8, 2015

bsideup commented Sep 14, 2015

bsideup commented Sep 14, 2015

bsideup commented Sep 14, 2015

bsideup commented Sep 14, 2015

lclarkmichalek commented Sep 14, 2015

bsideup commented Sep 14, 2015

bsideup commented Oct 15, 2015

drewrobb commented Oct 16, 2015

lavcraft commented Oct 26, 2015

j1n6 commented Oct 27, 2015

bsideup commented Oct 27, 2015

lclarkmichalek commented Nov 12, 2015

bsideup commented Nov 12, 2015

lavcraft commented Nov 13, 2015

bsideup commented Nov 13, 2015

lclarkmichalek commented Nov 13, 2015

bsideup commented Nov 13, 2015