Continuation of the unit state implementation.

The unit state isn't yet finalized. This branch contains
more methods regarding upgrade, resolved and open ports.
Additionally the replacement of ZooKeeper connections 
inside the entities by references to the state has been
continued.

https://code.launchpad.net/~themue/juju/go-state-continued-unit/+merge/95684

(do not edit description out of merge proposal)

[TheMue, 2012-03-02 22:30:55]

Please take a look.

[fwereade, 2012-03-09 11:48:02]

https://codereview.appspot.com/5727045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode218
state/unit.go:218: return retryTopologyChange(u.st.zk, unassignUnit)
Unrelated to this merge; but wouldn't it make sense for retryTopologyChange to
be a State method?

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode221
state/unit.go:221: // Upgrade returns if the upgrade flag is set.
Upgrade is a bit verby for my taste. NeedsUpgrade, perhaps?

Assume identical comment for SetUpgrade, zkUpgradePath, any others I've not
mentioned explicitly.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode264
state/unit.go:264: }
Would be good to verify we actually have either ResolvedRetryHooks or
ResolvedNoHooks and return an error if we don't.

I'm also not sure about the value of returning a map[string]interface{} when we
could just return ResolvedNoHooks, ResolvedRetryHooks, or (new) NotResolved
(=0). Seems like it'd be much nicer for client code?

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode274
state/unit.go:274: }
Maybe move this block out into `func validResolvedMode` or something (for reuse
above).

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode313
state/unit.go:313: goto found
This may just be goto-considered-harmful dogma -- the code isn't hard to follow
-- but I don't see that this is any better than a `found = true; break` and a
followup `if !found` block.

Apart fromanything else, the "found" label doesn't quite mean "found", it means
"once you're here, open now contains portProto, and it may or may not have been
found in the original data".

[rog, 2012-03-09 15:05:20]

https://codereview.appspot.com/5727045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode18
state/unit.go:18: ResolvedRetryHooks = 1000
i think these should get their own type. then the value to be passed to
SetResolved is more obvious.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode24
state/unit.go:24: type PortProto struct {
i think that just "Port" would be good here.
// Port identifies a network port for a particular protocol.

after all, we have OpenPort not OpenPortProto.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode218
state/unit.go:218: return retryTopologyChange(u.st.zk, unassignUnit)
On 2012/03/09 11:48:02, fwereade wrote:
> Unrelated to this merge; but wouldn't it make sense for retryTopologyChange to
> be a State method?

+1

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode221
state/unit.go:221: // Upgrade returns if the upgrade flag is set.
On 2012/03/09 11:48:02, fwereade wrote:
> Upgrade is a bit verby for my taste. NeedsUpgrade, perhaps?
> 
> Assume identical comment for SetUpgrade, zkUpgradePath, any others I've not
> mentioned explicitly.

+1

// NeedsUpgrade returns whether the unit needs an upgrade.
?

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode264
state/unit.go:264: }
On 2012/03/09 11:48:02, fwereade wrote:
> Would be good to verify we actually have either ResolvedRetryHooks or
> ResolvedNoHooks and return an error if we don't.
> 
> I'm also not sure about the value of returning a map[string]interface{} when
we
> could just return ResolvedNoHooks, ResolvedRetryHooks, or (new) NotResolved
> (=0). Seems like it'd be much nicer for client code?

+1

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode308
state/unit.go:308: }
the three preceding lines are unnecessary.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode311
state/unit.go:311: for i := range open {
for _, p := range open {

would be slightly clearer i think. then the "open" variable is probably
superfluous.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode313
state/unit.go:313: goto found
On 2012/03/09 11:48:02, fwereade wrote:
> This may just be goto-considered-harmful dogma -- the code isn't hard to
follow
> -- but I don't see that this is any better than a `found = true; break` and a
> followup `if !found` block.
> 
> Apart fromanything else, the "found" label doesn't quite mean "found", it
means
> "once you're here, open now contains portProto, and it may or may not have
been
> found in the original data".

+1

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode338
state/unit.go:338: }
-2,.d

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode342
state/unit.go:342: changedOpen := make([]PortProto, 0)
this could be:
var changedOpen []PortProto
(my preference)
or

changedOpen := []PortProto{}

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode363
state/unit.go:363: return []PortProto{}, nil
return nil, nil

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode368
state/unit.go:368: data := make(map[string][]PortProto)
you don't need the make here.
var data map[string][]PortProto
is fine if you use &data in the Unmarshal below.

i don't feel strongly about it though.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode372
state/unit.go:372: open := data["open"]
return data["open"]

a nil slice is an empty slice.

[TheMue, 2012-03-09 15:26:43]

Most comments are fine and will rush in with the next propose.

https://codereview.appspot.com/5727045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode18
state/unit.go:18: ResolvedRetryHooks = 1000
On 2012/03/09 15:05:20, rog wrote:
> i think these should get their own type. then the value to be passed to
> SetResolved is more obvious.

The values are todays values stored in ZK. They should be kept to be compatible.
But an own type would indeed make the function signatures more clear.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode218
state/unit.go:218: return retryTopologyChange(u.st.zk, unassignUnit)
On 2012/03/09 15:05:20, rog wrote:
> On 2012/03/09 11:48:02, fwereade wrote:
> > Unrelated to this merge; but wouldn't it make sense for retryTopologyChange
to
> > be a State method?
> 
> +1

Yep, sounds reasonable. But will be an extra branch.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode264
state/unit.go:264: }
On 2012/03/09 15:05:20, rog wrote:
> On 2012/03/09 11:48:02, fwereade wrote:
> > Would be good to verify we actually have either ResolvedRetryHooks or
> > ResolvedNoHooks and return an error if we don't.
> > 
> > I'm also not sure about the value of returning a map[string]interface{} when
> we
> > could just return ResolvedNoHooks, ResolvedRetryHooks, or (new) NotResolved
> > (=0). Seems like it'd be much nicer for client code?
> 
> +1

Will this node only contain the one retry information? Haven't been sure yet,
but in this case the change makes sense.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode274
state/unit.go:274: }
On 2012/03/09 11:48:02, fwereade wrote:
> Maybe move this block out into `func validResolvedMode` or something (for
reuse
> above).

Would do this when we have multiple usages. But found only this one so far.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode308
state/unit.go:308: }
On 2012/03/09 15:05:20, rog wrote:
> the three preceding lines are unnecessary.

Argh, yes, are remnants of a former approach.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode342
state/unit.go:342: changedOpen := make([]PortProto, 0)
On 2012/03/09 15:05:20, rog wrote:
> this could be:
> var changedOpen []PortProto
> (my preference)
> or
> 
> changedOpen := []PortProto{}

Why no more make? Is it deprecated?

[fwereade, 2012-03-09 15:38:49]

https://codereview.appspot.com/5727045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode264
state/unit.go:264: }
On 2012/03/09 15:26:43, TheMue wrote:
> On 2012/03/09 15:05:20, rog wrote:
> > On 2012/03/09 11:48:02, fwereade wrote:
> > > Would be good to verify we actually have either ResolvedRetryHooks or
> > > ResolvedNoHooks and return an error if we don't.
> > > 
> > > I'm also not sure about the value of returning a map[string]interface{}
when
> > we
> > > could just return ResolvedNoHooks, ResolvedRetryHooks, or (new)
NotResolved
> > > (=0). Seems like it'd be much nicer for client code?
> > 
> > +1
> 
> Will this node only contain the one retry information? Haven't been sure yet,
> but in this case the change makes sense.

AFAI can predict, that's all we'll have; at present the only reason to return a
map is to match an internal python interface; and if we do ever need to change
it it won't be too hard ;).

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode274
state/unit.go:274: }
On 2012/03/09 15:26:43, TheMue wrote:
> On 2012/03/09 11:48:02, fwereade wrote:
> > Maybe move this block out into `func validResolvedMode` or something (for
> reuse
> > above).
> 
> Would do this when we have multiple usages. But found only this one so far.

Previous comment suggests validation on output as well as input; hence 2 uses:).

[rog, 2012-03-09 16:20:28]

https://codereview.appspot.com/5727045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode342
state/unit.go:342: changedOpen := make([]PortProto, 0)
On 2012/03/09 15:26:43, TheMue wrote:
> On 2012/03/09 15:05:20, rog wrote:
> > this could be:
> > var changedOpen []PortProto
> > (my preference)
> > or
> > 
> > changedOpen := []PortProto{}
> 
> Why no more make? Is it deprecated?

it's fine, just a little more verbose than necessary.

[TheMue, 2012-03-09 18:37:02]

Please take a look.

[TheMue, 2012-03-09 18:43:45]

https://codereview.appspot.com/5727045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode18
state/unit.go:18: ResolvedRetryHooks = 1000
On 2012/03/09 15:05:20, rog wrote:
> i think these should get their own type. then the value to be passed to
> SetResolved is more obvious.

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode24
state/unit.go:24: type PortProto struct {
On 2012/03/09 15:05:20, rog wrote:
> i think that just "Port" would be good here.
> // Port identifies a network port for a particular protocol.
> 
> after all, we have OpenPort not OpenPortProto.

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode221
state/unit.go:221: // Upgrade returns if the upgrade flag is set.
On 2012/03/09 11:48:02, fwereade wrote:
> Upgrade is a bit verby for my taste. NeedsUpgrade, perhaps?
> 
> Assume identical comment for SetUpgrade, zkUpgradePath, any others I've not
> mentioned explicitly.

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode221
state/unit.go:221: // Upgrade returns if the upgrade flag is set.
On 2012/03/09 15:05:20, rog wrote:
> On 2012/03/09 11:48:02, fwereade wrote:
> > Upgrade is a bit verby for my taste. NeedsUpgrade, perhaps?
> > 
> > Assume identical comment for SetUpgrade, zkUpgradePath, any others I've not
> > mentioned explicitly.
> 
> +1
> 
> // NeedsUpgrade returns whether the unit needs an upgrade.
> ?

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode264
state/unit.go:264: }
On 2012/03/09 15:38:50, fwereade wrote:
> On 2012/03/09 15:26:43, TheMue wrote:
> > On 2012/03/09 15:05:20, rog wrote:
> > > On 2012/03/09 11:48:02, fwereade wrote:
> > > > Would be good to verify we actually have either ResolvedRetryHooks or
> > > > ResolvedNoHooks and return an error if we don't.
> > > > 
> > > > I'm also not sure about the value of returning a map[string]interface{}
> when
> > > we
> > > > could just return ResolvedNoHooks, ResolvedRetryHooks, or (new)
> NotResolved
> > > > (=0). Seems like it'd be much nicer for client code?
> > > 
> > > +1
> > 
> > Will this node only contain the one retry information? Haven't been sure
yet,
> > but in this case the change makes sense.
> 
> AFAI can predict, that's all we'll have; at present the only reason to return
a
> map is to match an internal python interface; and if we do ever need to change
> it it won't be too hard ;).

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode274
state/unit.go:274: }
On 2012/03/09 15:38:50, fwereade wrote:
> On 2012/03/09 15:26:43, TheMue wrote:
> > On 2012/03/09 11:48:02, fwereade wrote:
> > > Maybe move this block out into `func validResolvedMode` or something (for
> > reuse
> > > above).
> > 
> > Would do this when we have multiple usages. But found only this one so far.
> 
> Previous comment suggests validation on output as well as input; hence 2
uses:).

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode308
state/unit.go:308: }
On 2012/03/09 15:26:43, TheMue wrote:
> On 2012/03/09 15:05:20, rog wrote:
> > the three preceding lines are unnecessary.
> 
> Argh, yes, are remnants of a former approach.

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode311
state/unit.go:311: for i := range open {
On 2012/03/09 15:05:20, rog wrote:
> for _, p := range open {
> 
> would be slightly clearer i think. then the "open" variable is probably
> superfluous.

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode313
state/unit.go:313: goto found
On 2012/03/09 11:48:02, fwereade wrote:
> This may just be goto-considered-harmful dogma -- the code isn't hard to
follow
> -- but I don't see that this is any better than a `found = true; break` and a
> followup `if !found` block.
> 
> Apart fromanything else, the "found" label doesn't quite mean "found", it
means
> "once you're here, open now contains portProto, and it may or may not have
been
> found in the original data".

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode338
state/unit.go:338: }
On 2012/03/09 15:05:20, rog wrote:
> -2,.d

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode342
state/unit.go:342: changedOpen := make([]PortProto, 0)
On 2012/03/09 15:05:20, rog wrote:
> this could be:
> var changedOpen []PortProto
> (my preference)
> or
> 
> changedOpen := []PortProto{}

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode363
state/unit.go:363: return []PortProto{}, nil
On 2012/03/09 15:05:20, rog wrote:
> return nil, nil

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode368
state/unit.go:368: data := make(map[string][]PortProto)
On 2012/03/09 15:05:20, rog wrote:
> you don't need the make here.
> var data map[string][]PortProto
> is fine if you use &data in the Unmarshal below.
> 
> i don't feel strongly about it though.

Done.

https://codereview.appspot.com/5727045/diff/1/state/unit.go#newcode372
state/unit.go:372: open := data["open"]
On 2012/03/09 15:05:20, rog wrote:
> return data["open"]
> 
> a nil slice is an empty slice.

Done.

[fwereade, 2012-03-10 17:35:32]

Basically LGTM, but I'd like the chance to talk about naming a little; grab me
on IRC please.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode18
state/unit.go:18: // variable inside
incomplete doc comment

(see following comment though)

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode22
state/unit.go:22: ResolvedNoValue    RetryMode = -1
I kinda feel 0 would be better, but I'm open to arguments.

I'm also wondering whether we could come up with names that reflect what's going
on a little more clearly:

type ErrorResolution int
const (
  NotResolved = 0
  Retry       = 1000
  Resolved    = 1001
)

...but I'm not *really* sure it's much of an improvement. On reflection, I think
RetryMode was a bad suggestion (sorry); "resolution" is the best noun I can
think of *today* but I guess that's not much of a guarantee.

Grab me on IRC for a chat perhaps?

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode339
state/unit.go:339: return string(changedYaml), nil
Just a suggestion: oldYaml and newYaml might be good names to use in retry
functions generally.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode354
state/unit.go:354: portProto := Port{port, proto}
Similarly, oldOpen/newOpen might make it easier for me to follow this; and
closedPort (rather than portProto) would be nicely consistent with OpenPort.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode398
state/unit.go:398: // Name returns the name of the unit based on the service
Not sure where this is from :)

[TheMue, 2012-03-12 14:38:12]

Please take a look.

[TheMue, 2012-03-12 14:42:33]

PTAL

https://codereview.appspot.com/5727045/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode18
state/unit.go:18: // variable inside
On 2012/03/10 17:35:32, fwereade wrote:
> incomplete doc comment
> 
> (see following comment though)

Done.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode22
state/unit.go:22: ResolvedNoValue    RetryMode = -1
On 2012/03/10 17:35:32, fwereade wrote:
> I kinda feel 0 would be better, but I'm open to arguments.
> 
> I'm also wondering whether we could come up with names that reflect what's
going
> on a little more clearly:
> 
> type ErrorResolution int
> const (
>   NotResolved = 0
>   Retry       = 1000
>   Resolved    = 1001
> )
> 
> ...but I'm not *really* sure it's much of an improvement. On reflection, I
think
> RetryMode was a bad suggestion (sorry); "resolution" is the best noun I can
> think of *today* but I guess that's not much of a guarantee.
> 
> Grab me on IRC for a chat perhaps?

Done.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode339
state/unit.go:339: return string(changedYaml), nil
On 2012/03/10 17:35:32, fwereade wrote:
> Just a suggestion: oldYaml and newYaml might be good names to use in retry
> functions generally.

Done.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode354
state/unit.go:354: portProto := Port{port, proto}
On 2012/03/10 17:35:32, fwereade wrote:
> Similarly, oldOpen/newOpen might make it easier for me to follow this; and
> closedPort (rather than portProto) would be nicely consistent with OpenPort.

Done.

https://codereview.appspot.com/5727045/diff/7001/state/unit.go#newcode398
state/unit.go:398: // Name returns the name of the unit based on the service
On 2012/03/10 17:35:32, fwereade wrote:
> Not sure where this is from :)

Done.

[niemeyer, 2012-03-12 15:38:38]

Looks pretty good. Some minor details only:

https://codereview.appspot.com/5727045/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode19
state/unit.go:19: type ErrorResolution int
s/ErrorResolution/ResolvedMode/

Then, please use these names, respectively:

ResolvedNone
ResolvedRetryHooks
ResolvedNoHooks

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode22
state/unit.go:22: NotResolved ErrorResolution = -1
0 (zero) is a better constant for "unset".

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode30
state/unit.go:30: Proto string
Please use this here:

type Port struct {
    Protocol string `yaml:"proto"`
    Number   int    `yaml:"port"`
}

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode234
state/unit.go:234: // SetNeedsUpgrade informs the unit that it should perform an
upgrde.
s/upgrde/upgrade/

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode244
state/unit.go:244: // ClearNeedsUpgrade resets the upgrade notification. Itis
typically
s/Itis/It is/

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode257
state/unit.go:257: func (u *Unit) validRetryMode(er ErrorResolution) error {
Please use "mode" as the variable for ResolvedMode (it's not an "error
resolution" setting per se, it's already been resolved before).

Also, this is a function given that "u" is untouched below.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode274
state/unit.go:274: setting := make(map[string]ErrorResolution)
You don't need a map here. Please use this instead:

setting := &struct{ Retry ResolvedMode }{}

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode279
state/unit.go:279: // due to legacy reasons.
Please drop comment. This statement is true for every single data file we touch
in zookeeper.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode287
state/unit.go:287: // SetResolved marks the unit as in need of being resolved.
The
I know this is coming from the original code, but this comment is bogus.

Can you please replace it by:

// SetResolved marks the unit as having had any previous state
// transition problems resolved, and informs the unit that it may
// attempt to reestablish normal workflow.
// The resolved mode parameter informs whether to attempt to 
// reexecute previous failed hooks or to continue as if they had
// succeeded before.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode295
state/unit.go:295: // due to legacy reasons.
Same as before.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode296
state/unit.go:296: setting := map[string]ErrorResolution{"retry": er}
setting := &struct{ Retry ResolvedMode }{}

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode318
state/unit.go:318: // OpenPort sets the policy that port (using proto) should be
opened.
// OpenPort sets the policy of the port with protocol and number to open.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode319
state/unit.go:319: func (u *Unit) OpenPort(port int, proto string) error {
The port is only relevant within "protocol", so let's please invert the
parameters, and fix the parameter name:

    OpenPort(protocol string, number int)

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode321
state/unit.go:321: ports := map[string][]Port{}
ports := &struct{ Open []Port }{}

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode349
state/unit.go:349: func (u *Unit) ClosePort(port int, proto string) error {
Please adapt similarly to OpenPort regarding comment, parameters, and the use of
struct below.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode385
state/unit.go:385: var data map[string][]Port
struct

[TheMue, 2012-03-12 17:36:55]

Please take a look.

[niemeyer, 2012-03-12 18:13:14]

LGTM. Just a few extra trivials before submitting please:

https://codereview.appspot.com/5727045/diff/9003/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode255
state/unit.go:255: // ResolvedNoHooks returns the value of the resolved setting
if any.
// Resolved returns the resolved mode for the unit.

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode286
state/unit.go:286: setting := &struct{ Retry ResolvedMode }{Retry: mode}
s/Retry: //

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode308
state/unit.go:308: // OpenPort sets the policy of the port with protocol and
number to open.
s/open/opened/ (sorry, it was my mistake)

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode337
state/unit.go:337: // OpenPort sets the policy of the port with protocol and
number to close.
s/OpenPort/ClosePort/

Also, s/close/closed/ (my mistake again)

[TheMue, 2012-03-12 18:33:01]

https://codereview.appspot.com/5727045/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode19
state/unit.go:19: type ErrorResolution int
On 2012/03/12 15:38:39, niemeyer wrote:
> s/ErrorResolution/ResolvedMode/
> 
> Then, please use these names, respectively:
> 
> ResolvedNone
> ResolvedRetryHooks
> ResolvedNoHooks

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode22
state/unit.go:22: NotResolved ErrorResolution = -1
On 2012/03/12 15:38:39, niemeyer wrote:
> 0 (zero) is a better constant for "unset".

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode30
state/unit.go:30: Proto string
On 2012/03/12 15:38:39, niemeyer wrote:
> Please use this here:
> 
> type Port struct {
>     Protocol string `yaml:"proto"`
>     Number   int    `yaml:"port"`
> }

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode234
state/unit.go:234: // SetNeedsUpgrade informs the unit that it should perform an
upgrde.
On 2012/03/12 15:38:39, niemeyer wrote:
> s/upgrde/upgrade/

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode244
state/unit.go:244: // ClearNeedsUpgrade resets the upgrade notification. Itis
typically
On 2012/03/12 15:38:39, niemeyer wrote:
> s/Itis/It is/

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode257
state/unit.go:257: func (u *Unit) validRetryMode(er ErrorResolution) error {
On 2012/03/12 15:38:39, niemeyer wrote:
> Please use "mode" as the variable for ResolvedMode (it's not an "error
> resolution" setting per se, it's already been resolved before).
> 
> Also, this is a function given that "u" is untouched below.

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode274
state/unit.go:274: setting := make(map[string]ErrorResolution)
On 2012/03/12 15:38:39, niemeyer wrote:
> You don't need a map here. Please use this instead:
> 
> setting := &struct{ Retry ResolvedMode }{}

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode279
state/unit.go:279: // due to legacy reasons.
On 2012/03/12 15:38:39, niemeyer wrote:
> Please drop comment. This statement is true for every single data file we
touch
> in zookeeper.

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode287
state/unit.go:287: // SetResolved marks the unit as in need of being resolved.
The
On 2012/03/12 15:38:39, niemeyer wrote:
> I know this is coming from the original code, but this comment is bogus.
> 
> Can you please replace it by:
> 
> // SetResolved marks the unit as having had any previous state
> // transition problems resolved, and informs the unit that it may
> // attempt to reestablish normal workflow.
> // The resolved mode parameter informs whether to attempt to 
> // reexecute previous failed hooks or to continue as if they had
> // succeeded before.

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode295
state/unit.go:295: // due to legacy reasons.
On 2012/03/12 15:38:39, niemeyer wrote:
> Same as before.

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode296
state/unit.go:296: setting := map[string]ErrorResolution{"retry": er}
On 2012/03/12 15:38:39, niemeyer wrote:
> setting := &struct{ Retry ResolvedMode }{}

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode318
state/unit.go:318: // OpenPort sets the policy that port (using proto) should be
opened.
On 2012/03/12 15:38:39, niemeyer wrote:
> // OpenPort sets the policy of the port with protocol and number to open.

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode319
state/unit.go:319: func (u *Unit) OpenPort(port int, proto string) error {
On 2012/03/12 15:38:39, niemeyer wrote:
> The port is only relevant within "protocol", so let's please invert the
> parameters, and fix the parameter name:
> 
>     OpenPort(protocol string, number int)

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode321
state/unit.go:321: ports := map[string][]Port{}
On 2012/03/12 15:38:39, niemeyer wrote:
> ports := &struct{ Open []Port }{}

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode349
state/unit.go:349: func (u *Unit) ClosePort(port int, proto string) error {
On 2012/03/12 15:38:39, niemeyer wrote:
> Please adapt similarly to OpenPort regarding comment, parameters, and the use
of
> struct below.

Done.

https://codereview.appspot.com/5727045/diff/11001/state/unit.go#newcode385
state/unit.go:385: var data map[string][]Port
On 2012/03/12 15:38:39, niemeyer wrote:
> struct

Done.

https://codereview.appspot.com/5727045/diff/9003/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode255
state/unit.go:255: // ResolvedNoHooks returns the value of the resolved setting
if any.
On 2012/03/12 18:13:15, niemeyer wrote:
> // Resolved returns the resolved mode for the unit.

Done.

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode286
state/unit.go:286: setting := &struct{ Retry ResolvedMode }{Retry: mode}
On 2012/03/12 18:13:15, niemeyer wrote:
> s/Retry: //

Done.

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode308
state/unit.go:308: // OpenPort sets the policy of the port with protocol and
number to open.
On 2012/03/12 18:13:15, niemeyer wrote:
> s/open/opened/ (sorry, it was my mistake)

Done.

https://codereview.appspot.com/5727045/diff/9003/state/unit.go#newcode337
state/unit.go:337: // OpenPort sets the policy of the port with protocol and
number to close.
On 2012/03/12 18:13:15, niemeyer wrote:
> s/OpenPort/ClosePort/
> 
> Also, s/close/closed/ (my mistake again)

Done.

[TheMue, 2012-03-12 18:34:49]

Please take a look.

[TheMue, 2012-03-12 19:02:01]

Please take a look.

[TheMue, 2012-03-12 19:07:18]

*** Submitted:

Continuation of the unit state implementation.

The unit state isn't yet finalized. This branch contains
more methods regarding upgrade, resolved and open ports.
Additionally the replacement of ZooKeeper connections 
inside the entities by references to the state has been
continued.

R=fwereade, rog, niemeyer
CC=
https://codereview.appspot.com/5727045