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

Issue 6864050: state: Dying unit cannot enter relation scope

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 1 month ago by fwereade
Modified:
9 years, 1 month ago
Reviewers:
mp+137585
Visibility:
Public.

Description

state: Dying unit cannot enter relation scope https://code.launchpad.net/~fwereade/juju-core/dying-unit-enter-scope/+merge/137585 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 14

Patch Set 2 : state: Dying unit cannot enter relation scope #

Total comments: 3

Patch Set 3 : state: Dying unit cannot enter relation scope #

Unified diffs Side-by-side diffs Delta from patch set Stats (+72 lines, -32 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M state/relation.go View 1 3 chunks +38 lines, -14 lines 0 comments Download
M state/relation_test.go View 6 chunks +20 lines, -6 lines 0 comments Download
M worker/uniter/uniter.go View 2 chunks +12 lines, -12 lines 0 comments Download

Messages

Total messages: 16
fwereade
Please take a look.
9 years, 1 month ago (2012-12-03 14:51:30 UTC) #1
jameinel
I'm a little concerned about how this is being glued together. It at least looks ...
9 years, 1 month ago (2012-12-04 09:06:08 UTC) #2
fwereade
jameinel, thanks for your comments; I'd be happy to talk more about your concerns, but ...
9 years, 1 month ago (2012-12-04 11:31:44 UTC) #3
fwereade
On Tue, 2012-12-04 at 12:50 +0000, John A Meinel wrote: To start with, I'm fully ...
9 years, 1 month ago (2012-12-05 08:43:42 UTC) #4
fwereade
> In a SQL DB, you're doing some sort of UPDATE/INSERT that violates a > ...
9 years, 1 month ago (2012-12-05 10:37:34 UTC) #5
jameinel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 ... > Regardless, I think it's a very tricky ...
9 years, 1 month ago (2012-12-06 07:10:11 UTC) #6
fwereade
On 2012/12/05 10:37:34, fwereade wrote: > AFAICT there's no way to get any more nuance ...
9 years, 1 month ago (2012-12-07 09:45:07 UTC) #7
rog
looks plausible. a couple of comments below. https://codereview.appspot.com/6864050/diff/1/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6864050/diff/1/state/relation.go#newcode260 state/relation.go:260: // the ...
9 years, 1 month ago (2012-12-07 10:06:21 UTC) #8
TheMue
So far it's looking good, I only have a problem with the usage of an ...
9 years, 1 month ago (2012-12-07 10:07:09 UTC) #9
fwereade
A few responses... opinions? https://codereview.appspot.com/6864050/diff/1/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6864050/diff/1/state/relation.go#newcode293 state/relation.go:293: return fmt.Errorf("cannot initialize state for ...
9 years, 1 month ago (2012-12-07 10:30:04 UTC) #10
TheMue
On 2012/12/07 10:30:04, fwereade wrote: > A few responses... opinions? > I'm confused... why is ...
9 years, 1 month ago (2012-12-07 10:37:19 UTC) #11
rog
https://codereview.appspot.com/6864050/diff/1/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6864050/diff/1/state/relation.go#newcode293 state/relation.go:293: return fmt.Errorf("cannot initialize state for %s: %v", desc, err) ...
9 years, 1 month ago (2012-12-07 10:40:53 UTC) #12
fwereade
Please take a look. https://codereview.appspot.com/6864050/diff/1/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6864050/diff/1/state/relation.go#newcode260 state/relation.go:260: // the unit not to ...
9 years, 1 month ago (2012-12-07 15:06:11 UTC) #13
rog
LGTM https://codereview.appspot.com/6864050/diff/9002/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6864050/diff/9002/state/relation.go#newcode305 state/relation.go:305: return err nice
9 years, 1 month ago (2012-12-07 15:09:17 UTC) #14
TheMue
LGTM, only one small question. https://codereview.appspot.com/6864050/diff/9002/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6864050/diff/9002/state/relation.go#newcode328 state/relation.go:328: return fmt.Errorf("cannot enter scope ...
9 years, 1 month ago (2012-12-07 15:11:43 UTC) #15
fwereade
9 years, 1 month ago (2012-12-07 16:52:37 UTC) #16
*** Submitted:

state: Dying unit cannot enter relation scope

R=jameinel, TheMue, rog
CC=
https://codereview.appspot.com/6864050

https://codereview.appspot.com/6864050/diff/9002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6864050/diff/9002/state/relation.go#newcode328
state/relation.go:328: return fmt.Errorf("cannot enter scope for %s:
inconsistent state", desc)
On 2012/12/07 15:11:43, TheMue wrote:
> Is an error enough or isn't it worth a panic?

I don't think it's very helpful to panic -- we don't get any useful extra
information, and we'll take down the uniter anyway -- it'll either recover or
logspam us every 5 seconds, and I'd rather have concise logspam :).
Sign in to reply to this message.

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