Code review comment for lp://staging/~dave-cheney/pyjuju/go-provisioning-strawman

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Some more comments.

Also, it just occurred to me that, as a very first step getting just the
testing environment in place, with zero functionality, in a way that
reliably starts and stops, is already a fantastic task on itself.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#newcode53
cmd/jujud/provisioning.go:53: a.State, err =
state.Open(&a.Conf.StateInfo)
This is a rather extensive function that might be broken down further in
more manageable bits for clarity.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#newcode75
cmd/jujud/provisioning.go:75: // step 2. listen for changes to the
environment or the machine topology and action both.
Step 1 needs to be able to be more resilient, and step 2 must be able to
fallback to step 1 in case of issues with the connection. It must also
be able to deal with Stop of the watcher and re-watching, but that
should be somewhat natural.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#newcode122
cmd/jujud/provisioning.go:122: return fmt.Errorf("machine-%010d already
reports a provider id %q, skipping", m.Id(), id)
We should never talk about internal ZooKeeper keys to the user. This
should be saying "machine %d" instead.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#newcode134
cmd/jujud/provisioning.go:134: // store the reference from the provider
in ZK
Let's please not reference ZooKeeper all the time. It is the environment
state that we have at hand, with a nice abstraction that we created
precisely so we don't think in terms of ZooKeeper all the time.
ZooKeeper is hopefully going away soon, but the state will remain as-is.

https://codereview.appspot.com/6250068/

« Back to merge proposal