Code review comment for lp:~axwalk/juju-core/lp1271504-ensureavailability-agentalive
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
4d1350f
(Get the code!)
Please take a look.
https:/
File state/addmachine.go (right):
https:/
state/addmachin
On 2014/04/11 10:09:17, rog wrote:
> If err==txn.
> else has run EnsureAvailability concurrently. Perhaps we should check
if the
> voting server count has changed - if it has, return an error,
otherwise return
> nil because our work will already have been done.
Done, I made it a transaction loop and added a few tests.
https:/
state/addmachin
On 2014/04/11 10:09:17, rog wrote:
> rather than two variables, it might be clearer if we did:
> var newInfo StateServerInfo
> and then, e.g.
> newInfo.MachineIds = append(
> then it's more obvious when we're just building
> up ids in the new info.
Done.
https:/
state/addmachin
On 2014/04/11 10:09:17, rog wrote:
> switch the sense of the condition to avoid the negative?
Done.
https:/
state/addmachin
set novote and
On 2014/04/11 10:09:17, rog wrote:
> Since the comments are inside the if statement, we can phrase them
> unconditionally, which might read slightly more clearly.
> // The machine wants to vote, so we simply set novote etc
Done.
https:/
state/addmachin
On 2014/04/11 10:09:17, rog wrote:
> perhaps invert the condition here to save negatives?
Done.
https:/
state/addmachin
doesn't have a vote,
On 2014/04/11 10:09:17, rog wrote:
> again, we can phrase it unconditionally.
Done.
https:/
state/addmachin
false}}}},
On 2014/04/11 10:09:17, rog wrote:
> I *think* this should be {{"$eq", true}}, because nil!=false
> and legacy machines may have a nil novote field.
> similarly below.
Done.
https:/
state/addmachin
JobManageEnviro
On 2014/04/11 10:09:17, rog wrote:
> perhaps set novote=false here too?
Done.
https:/