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

Issue 6575053: uniter: lifecycle handling

Can't Edit
Can't Publish+Mail
Start Review
10 years, 2 months ago by fwereade
10 years, 2 months ago
niemeyer, mp+126558


uniter: lifecycle handling This remains WIP just for the moment, largely because it lacks tests (ha!). Things to note and perhaps comment on at this stage: * The modeStarting type is, I *think*, a Good Thing, because it allows me to focus on smaller parts of the problem at a time. I believe it will start to really pull its weight when I add relations; unless you feel there's something fundamentally wrong with the concept/approach, please be indulgent of its existence for a CL or two :). * Man, lifecycle handling is a lot of code. This is particularly noticeable in ModeHookError and ModeConflicted, which now share enough lifecycle and watch manipulation logic that I'm very tempted to carve out a parametrized modeAwaitResolved to keep it in. I'm really not sure whether this is wantonly premature abstraction or sensible factoring of fiddly logic into one place only, but I kinda still lean towards the former; this would most probably change if we got *another* distinct error mode. ...but, really, anything that springs to mind is fair game; absent comments, I expect to propose something pretty close to this at some stage tomorrow. https://code.launchpad.net/~fwereade/juju-core/uniter-unit-lifecycle/+merge/126558 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1

Patch Set 2 : uniter: lifecycle handling #

Patch Set 3 : uniter: lifecycle handling #

Unified diffs Side-by-side diffs Delta from patch set Stats (+264 lines, -78 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M worker/uniter/modes.go View 1 8 chunks +255 lines, -78 lines 0 comments Download
M worker/uniter/uniter.go View 2 chunks +7 lines, -0 lines 0 comments Download


Total messages: 2
Please take a look. https://codereview.appspot.com/6575053/diff/1/worker/uniter/modes.go File worker/uniter/modes.go (right): https://codereview.appspot.com/6575053/diff/1/worker/uniter/modes.go#newcode214 worker/uniter/modes.go:214: // ...otherwise, shut down. This ...
10 years, 2 months ago (2012-09-26 22:00:08 UTC) #1
10 years, 2 months ago (2012-09-27 01:04:50 UTC) #2
This is a great code spike. It enables us to have a pretty clear picture of what
has to be done and where. It also gives some comfort that the mode concept is
flexible enough to handle further abuse.

That said, I think it's time to step back and clean it up a bit by understanding
what we're doing. We have a lot of logic repetition, we have state machines
within the state machine (only watch bar after the first event of watching foo),
we're handling events which are global in a local way (either the whole unit is
supposed to die or not), etc.

I think pretty much every watcher we have can be setup upfront, and be made
available via the Uniter interface. We also don't have to offer the modes such a
low-level interface. A few examples to get your mind ticking:

    case <-uniter.tombDying():

This is closed when the unit tomb dies. It's probably just a dumb method that
returns u.tomb.Dying(), but it's good to follow the pattern.

    case <-uniter.unitDying():

This is closed when either the service or the unit state change and become

    case ch := <-uniter.charmUpgrade():

This fires once every time the service properties are found to be modified (via
a watcher) *and* the charm URL is found to have changed.

    case config := <-uniter.configChanged():

This fires once every time the service configuration has changed. To support it,
we'd also have:

    config := <-uniter.currentConfig()

in the same loop that feeds configChanged. This means we avoid the current back
and forth to handle the config-changed hook, and can manipulate its execution

None of these channels should be closed, so we don't have to  worry about ok

You see where this is going? Except for tombDying none of these channels *have*
to be consumed, which means it's totally fine for a mode to care only about its
own business and ignore everything else that it's someone else's problem.

I suspect this would not only avoid the significant burden that the code spike
is currently exposing by the introduction of lifecycle support, but it'd also
simplify the currently existing logic.
Sign in to reply to this message.

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