Code review - Issue 5727045: Continuation of the unit state implementation.https://codereview.appspot.com/2012-03-12T19:07:18+00:00rietveld
Message from unknown
2012-03-02T22:30:48+00:00TheMueurn:md5:a3932fba3f509c5a4472ef1445573cde
Message from themue@gmail.com
2012-03-02T22:30:55+00:00TheMueurn:md5:47da4fd484836c1697fb57744de933c6
Please take a look.
Message from fwereade@gmail.com
2012-03-09T11:48:02+00:00fwereadeurn:md5:d95d99c930c3f4b21d34cf0419929faa
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".
Message from rogpeppe@gmail.com
2012-03-09T15:05:20+00:00rogurn:md5:a630b7e1026ac5a5de1eea4d19b293b9
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.
Message from themue@gmail.com
2012-03-09T15:26:43+00:00TheMueurn:md5:7133ae9be488c661a059fbd58093ad28
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?
Message from fwereade@gmail.com
2012-03-09T15:38:49+00:00fwereadeurn:md5:ef195b96f281ba47d51628d7d28f1458
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:).
Message from rogpeppe@gmail.com
2012-03-09T16:20:28+00:00rogurn:md5:b53937d4f7c2cc746be7e2512cf5ca73
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.
Message from unknown
2012-03-09T18:36:59+00:00TheMueurn:md5:09da36c470f721af6bccc9bba763e50c
Message from themue@gmail.com
2012-03-09T18:37:02+00:00TheMueurn:md5:f9214c4b618e35c2e54b61a15a8c1c8a
Please take a look.
Message from themue@gmail.com
2012-03-09T18:43:45+00:00TheMueurn:md5:54bacf842f236121a1b1300614f45423
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.
Message from fwereade@gmail.com
2012-03-10T17:35:32+00:00fwereadeurn:md5:cfc524eab54357e608a63f1403c59a2b
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 :)
Message from unknown
2012-03-12T14:38:10+00:00TheMueurn:md5:dba3c7abc147b9a5289fb778c96ee250
Message from themue@gmail.com
2012-03-12T14:38:12+00:00TheMueurn:md5:d4ce8fc6bfaf20dcd2e169c60697ccce
Please take a look.
Message from themue@gmail.com
2012-03-12T14:42:33+00:00TheMueurn:md5:00589b026a4b103f312a108dc70827c6
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.
Message from n13m3y3r@gmail.com
2012-03-12T15:38:38+00:00niemeyerurn:md5:8c8a51c38240dd72e75030065143b62e
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
Message from unknown
2012-03-12T17:36:48+00:00TheMueurn:md5:735947f5858641e6cd870f8bd90c4c43
Message from themue@gmail.com
2012-03-12T17:36:55+00:00TheMueurn:md5:a106582c21d533bd9c4dcd8346fba77c
Please take a look.
Message from n13m3y3r@gmail.com
2012-03-12T18:13:14+00:00niemeyerurn:md5:2f2780aadf5b078b1adc3d781e99940d
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)
Message from themue@gmail.com
2012-03-12T18:33:01+00:00TheMueurn:md5:a8255507a19122d15bd942f908ca898d
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.
Message from unknown
2012-03-12T18:34:47+00:00TheMueurn:md5:88d1c7c549c78e520a3c6166d7146f92
Message from themue@gmail.com
2012-03-12T18:34:49+00:00TheMueurn:md5:0e6ecc74f4f8b134ad3bde018a22d837
Please take a look.
Message from unknown
2012-03-12T19:01:59+00:00TheMueurn:md5:2755de67efe4dd7711f56d7736557d20
Message from themue@gmail.com
2012-03-12T19:02:01+00:00TheMueurn:md5:98ed3db914e62fe3fd898afe1ed6030e
Please take a look.
Message from unknown
2012-03-12T19:06:51+00:00TheMueurn:md5:abfd61099c35cb2a0c9a39f499db35c7
Message from themue@gmail.com
2012-03-12T19:07:18+00:00TheMueurn:md5:4a685b3334118bf6f5957e24bc8a3a1a
*** 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