WIP

WIP



https://code.launchpad.net/~fwereade/juju-core/uniter-filter-watches/+merge/126751

(do not edit description out of merge proposal)

[niemeyer, 2012-09-27 18:16:58]

Woah, this is so much better indeed!

I'm making a few comments below to help on your WIPing, but overall it's
excellent, thank you.

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

https://codereview.appspot.com/6568060/diff/1/worker/uniter/modes.go#newcode176
worker/uniter/modes.go:176: if err == errNoUpgrade {
This same logic is repeated in three different places. What about:

    case charm := <-u.charmUpgrades(false):

?

https://codereview.appspot.com/6568060/diff/1/worker/uniter/modes.go#newcode225
worker/uniter/modes.go:225: }
The logic below makes me feel like there are two different modes merged
together. It should be easy to make a decision here and return a different mode
based on it, so that we're not manipulate what the loop actually does or not
based on what we've seen (which makes reasoning harder).

https://codereview.appspot.com/6568060/diff/1/worker/uniter/modes.go#newcode244
worker/uniter/modes.go:244: if err = u.resolveError(func(retry bool) error {
Pretty hard to create a good mind picture of he whole back and forth here. Why
isn't the retry mode returned from the watcher itself, and then we just need a
u.clearResolved() helper?

    case retry := <-u.resolvedChanges():
        ... foo ...
        err := u.clearResolved()

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

https://codereview.appspot.com/6568060/diff/1/worker/uniter/uniter.go#newcode313
worker/uniter/uniter.go:313: if err == nil || err == errHookFailed {
Code calls resolveError which calls code which returns an error which happens to
be a named result, which is caught up on a defer, which acts back on it, in some
cases, and changes the error, potentially.

I hope there's a still terse way to do the same in a more "forward" way.
Alternatively, I hope I don't have to debug it. :-)

[niemeyer, 2012-09-27 18:28:27]

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

https://codereview.appspot.com/6568060/diff/1/worker/uniter/filter.go#newcode68
worker/uniter/filter.go:68: resolved = unit.Resolved()
Looks like there's a race here. We can't make the decision of what the mode is
in a way independent from what the ModeFoos have seen. Otherwise, imagine what
happens if right after you send the tick, the other side loads it, we load it
here again, but the two values differ. We'll never send another note, despite
the fact that the other side hasn't seen the new value.

We can send the local knowledge of what the mode value is down the pipe to fix
it.

https://codereview.appspot.com/6568060/diff/1/worker/uniter/filter.go#newcode73
worker/uniter/filter.go:73: curl, force = service.CharmURL()
Ditto.