uniter: react sanely to Dying required subordinate

uniter: react sanely to Dying required subordinate

see lp:1091866

https://code.launchpad.net/~fwereade/juju-core/wait-dying-subordinates/+merge/142960

(do not edit description out of merge proposal)

[fwereade, 2013-01-11 17:59:04]

Please take a look.

[dimitern, 2013-01-11 18:07:07]

LGTM

[rog, 2013-01-14 17:37:40]

looks good. a few remarks below.

https://codereview.appspot.com/7094045/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7094045/diff/1/state/relation.go#newcode348
state/relation.go:348: if err :=
ru.st.units.Find(selSubordinate).Select(lifeFields).One(&doc); err != nil {
could we use the first Find to get this info and thereby avoid an extra round
trip to mongo?

https://codereview.appspot.com/7094045/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7094045/diff/1/worker/uniter/uniter.go#newcode436
worker/uniter/uniter.go:436: if err = r.Join(); err ==
state.ErrCannotEnterScopeYet {
s/=/:=/

(or ditch the else clause below)

https://codereview.appspot.com/7094045/diff/1/worker/uniter/uniter.go#newcode437
worker/uniter/uniter.go:437: log.Printf("worker/uniter: waiting for relation
scope to become accessible")
perhaps a comment might be in order here - it's
not very obvious why watching the unit will
ensure that we retry when we're able to enter scope.

something like this, perhaps?

// We get this error when a subordinate is dying but
// not yet dead. When the subordinate finally dies,
// we will see a watcher change (the set of subordinate
// names will change), so we retry whenever the unit changes.

[fwereade, 2013-01-14 22:35:59]

Please take a look.

https://codereview.appspot.com/7094045/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7094045/diff/1/state/relation.go#newcode348
state/relation.go:348: if err :=
ru.st.units.Find(selSubordinate).Select(lifeFields).One(&doc); err != nil {
On 2013/01/14 17:37:40, rog wrote:
> could we use the first Find to get this info and thereby avoid an extra round
> trip to mongo?

Yeah, nice, we can just call .One() once. Thanks. Done.

https://codereview.appspot.com/7094045/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7094045/diff/1/worker/uniter/uniter.go#newcode436
worker/uniter/uniter.go:436: if err = r.Join(); err ==
state.ErrCannotEnterScopeYet {
On 2013/01/14 17:37:40, rog wrote:
> s/=/:=/
> 
> (or ditch the else clause below)

Done.

https://codereview.appspot.com/7094045/diff/1/worker/uniter/uniter.go#newcode437
worker/uniter/uniter.go:437: log.Printf("worker/uniter: waiting for relation
scope to become accessible")
On 2013/01/14 17:37:40, rog wrote:
> perhaps a comment might be in order here - it's
> not very obvious why watching the unit will
> ensure that we retry when we're able to enter scope.
> 
> something like this, perhaps?
> 
> // We get this error when a subordinate is dying but
> // not yet dead. When the subordinate finally dies,
> // we will see a watcher change (the set of subordinate
> // names will change), so we retry whenever the unit changes.

I think it sort of works with just a logging tweak.

[rog, 2013-01-15 09:26:22]

LGTM with one remark below.

https://codereview.appspot.com/7094045/diff/7001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7094045/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:436: log.Printf("worker/uniter: cannot enter scope for
relation %q; waiting for subordinates to change", rel)
"waiting for subordinate removal"?

(that's what we're actually waiting for, i think)

[fwereade, 2013-01-15 11:37:55]

*** Submitted:

uniter: react sanely to Dying required subordinate

see lp:1091866

R=dimitern, rog
CC=
https://codereview.appspot.com/7094045

https://codereview.appspot.com/7094045/diff/7001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7094045/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:436: log.Printf("worker/uniter: cannot enter scope for
relation %q; waiting for subordinates to change", rel)
On 2013/01/15 09:26:22, rog wrote:
> "waiting for subordinate removal"?
> 
> (that's what we're actually waiting for, i think)

Done.