1088843 - taskcluster-client: PulseConnection and PulseListener need to support reconnecting

Status: RESOLVED DUPLICATE of bug 1335953

(Taskcluster :: Services, defect)

Description

[Jonas Finnemann Jensen (:jonasfj)]

Currently, we just crash whenever the AMQP connection fails.
This is a decent strategy... but we might want to retry the connection and send the unconfirmed messages again.

There needs to be some interaction between PulseListener and PulseConnection for this to work.

Ideally, we should also be able to use this logic (and PulseConnection) in taskcluster-base/exchanges so that we only need one AMQP connection per node and that one connection will reconnect if broken.

Comment 1

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: Github PR

This has been in the pipeline for a long time...
I think we should have the same retry-logic in go-client, not that we're in a hurry to implement it!
(IMO it needs some restructuring to support error handling)

---
Note, We shouldn't publish this until the taskcluster-base exchange/publisher utilities supports reconnect. But we should review it before we fix tc-base.

Comment 2

[Pete Moore [:pmoore][:pete]]

Thanks for doing this! Let's have a vidyo to go through it, as there is quite a lot of change there. This will help me prepare a better review.

Comment 3

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8654454 [details] [review]
Github PR

As discussed, let's have one of the other guys look at this, who will probably have more insights than me. Sorry for holding onto it for so long! :/

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Picking this back up again..

I see a few things to do here:

 * Abstract the pulse connection code out into a new library -- taking bits and pieces from both pulse-publisher and taskcluster-client
   * Add a callback for new connections so that users can (repeatedly) declare their exchanges, etc.
   * Otherwise use EventEmitter for a nice contained API
   * Support automatic reconnects, and even deliberate connection drops to exercise reconnect logic
   * Support various credentials
   * Support fake connections
 * Rewrite tc-client to use this library

For the moment, the Go client's pulse interface is unused, so I won't try to work on that.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Brian points out that most of the sentry exceptions are pulse-related, so even just cleaning those up to separate signal from noise would be a big win.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Consumer support in tc-lib-pulse: https://github.com/taskcluster/taskcluster-lib-pulse/pull/3

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

I'm going to drop this -- it's taking time I could be spending on redeployability.  It's certainly open for others to work on (perhaps Biboswan?).  There are some review comments to address in the PR.