/juju

api: API connection monitoring improvements #6398

Merged
merged 10 commits into from Oct 7, 2016

Conversation

Reviewers

@wallyworld wallyworld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mjs @jujubot @wallyworld
@mjs
Contributor

mjs commented Oct 7, 2016

The core change here is that an api.Connection's underlying rpc.Conn's Dead channel is now monitored. This means that a broken connection is now noticed much sooner than waiting for the next connection ping (pings are still used as a fallback however).

Other improvements were made along the way:

  • the connection monitoring logic was extracted into its own type and file
  • connection monitoring now has tests (completely untested before)
  • PingPeriod and PingTimeout are now consts
  • PingTimeout is no longer unnecessarily exported

mjs added some commits Oct 6, 2016

@mjs
api: Extract connection monitoring 
... and added missing test coverage.

Also fixed handling of the `closed` channel being closed. Another ping
was being unnecessarily attempted in this case.
2ec6cdb
@mjs
api: Use test helper for channel events
195a764
@mjs
api: Rearrange pings so they happen after timeout 
There's no need to ping just after the connection comes up, especially
with the upcoming dead monitoring in place.
3eafa6c
@mjs
api: waitThenAdvance helper 
Reduces test boilerplate.
0392b83
@mjs
api: Connection dies when underlying rpc.Conn dies 
Instead of relying solely on pings, api.Connection will now also die
early if the underlying rpc.Conn's Dead channel is closed. This allows
dead connections to be noticed much more quickly.
9a7ef9a
@mjs
api: Extracted monitoring to separate file
7ade363
@mjs
api: monitor now takes ping durations as args 
... rather than using global constants
5adfc84
@mjs
api: PingTimeout is no longer exported 
It should be exported and doesn't need to be.
a781825
@mjs
api: Log when RPC connection dies
4561cab
@wallyworld

wallyworld approved these changes Oct 7, 2016
View changes

Nice

api/apiclient.go
@@ -453,6 +460,10 @@ func (st *state) apiEndpoint(path, query string) (*url.URL, error) {
}, nil
}
+func (s *state) Ping() error {
@wallyworld

wallyworld Oct 7, 2016

Owner

comment

@mjs

mjs Oct 7, 2016

Contributor

It was pre-existing (I only moved it). I suspect it doesn't need to be exported so I'l aim for that.

@mjs
Contributor

mjs commented Oct 7, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 7, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit d7da116 into juju:master Oct 7, 2016

@mjs mjs deleted the mjs:apiclient-dead-detection branch Oct 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment