Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(280)

Issue 17700043: juju/api: don't warn on aborted connection

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by jameinel
Modified:
10 years, 5 months ago
Reviewers:
gz, mp+192789
Visibility:
Public.

Description

juju/api: don't warn on aborted connection I've been playing with Roger's tweak to using cached API addresses rather than determining them by reading the provider-state file. It works, but it has a bug that when it successfully connects to the API with the cached information, it causes the other goroutine to stop with an "aborted" exception. That was being displayed to the user as a WARNING which is pretty clearly incorrect. I can understand that we don't want to silently consume errors, but errAborted should be treated as control flow logic. So I made that happen. I wanted to add testing that we output sane information to the user, but it was clumsy to find the right hook points. If people feel strongly I can try to track that down, but I didn't want to lose the patch in flight. https://code.launchpad.net/~jameinel/juju-core/tweak-cached-address-functionality/+merge/192789 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -10 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M juju/api.go View 4 chunks +10 lines, -2 lines 1 comment Download
M juju/apiconn_test.go View 7 chunks +8 lines, -8 lines 0 comments Download

Messages

Total messages: 2
jameinel
Please take a look.
10 years, 6 months ago (2013-10-26 16:55:02 UTC) #1
gz
10 years, 5 months ago (2013-10-31 09:15:05 UTC) #2
LGTM.

https://codereview.appspot.com/17700043/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/17700043/diff/1/juju/api.go#newcode36
juju/api.go:36: var errAborted = fmt.Errorf("aborted")
Er... is this really the best way of doing this? I guess so.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b