mstate: state package replacement using mgo.

mstate: state package replacement using mgo.

This is the first step towards replacing Zookeeper with MongoDB.
Mstate will slowly reimplement the state package while preserving the
original state API.

This change implements preliminary support for Machines.

https://code.launchpad.net/~aramh/juju-core/mstate-machine-basic/+merge/111235

(do not edit description out of merge proposal)

[aram, 2012-06-18 21:59:18]

Please take a look.

[dave_cheney.net, 2012-06-19 05:51:19]

On 2012/06/18 21:59:18, aram wrote:
> Please take a look.

Looking good. 

On comment, and this is just my personal taste, but I feel you are overusing
named return arguments. I think they should be used sparingly, only where it
makes sense in deeply nested loops. I say this because of their potential to
shadow variables. I have no idea what the opinion of the group is on this point
of style so please take this as a simple a small grain of salt.

[dave_cheney.net, 2012-06-19 05:51:28]

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode20
mstate/machine.go:20: }
can we have a String() method as well please.

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode24
mstate/machine.go:24: mdoc := &machineDoc{}
var mdoc = new(machineDoc) ?

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go
File mstate/mgo_test.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode18
mstate/mgo_test.go:18: const mgoport = "27017" // 50017
const (
...
)

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode46
mstate/mgo_test.go:46: err := DropAll(mgoaddr)
if err := ...; err != nil {
   c.Fatalf("mongo dun goofed:%v", err)
}

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode54
mstate/mgo_test.go:54: panic(err)
ditto

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode69
mstate/mgo_test.go:69: time.Sleep(5e8)
Please use the time constants here, time is already imported. You can also
reused environs.Attempt.

[aram, 2012-06-19 10:48:21]

> On comment, and this is just my personal taste, but I feel you are overusing
> named return arguments.

I love named return arguments. They provide some nice things:
 1) Much easier refactoring. If I need to change or rearrange code
    inside a function I usually don't have to think how return
    parameters change.
 2) Proper usage of them forces the programmer to write code in a
    top to bottom fashion making the code easier to read and
    understand (Tob Duff of Duff's Device fame has a nice essey
    about this: http://iq0.com/notes/deep.nesting.html )
 3) They provide a nice mental model of what would be returned at
    all times making the code easy to reason about. When you start
    reading a function, you read the named return arguments first,
    then you read the function thinking "what happens to these return
    arguments and in what conditions". You always know the output, and
    this helps a lot.
 4) When you use named return arguments and do not return unadorned,
    but construct new return values, it's a big warning sign you can't
    miss. It's a mark that it should be read extra carefully.
 5) They also allow for stubs like this:

      func f() (s string, err error) { return }

    If I'd have to write many stubs like the above, things would
    get complicated for no reason (I've written a gostub command
    a few days ago:
    http://code.google.com/p/rbits/source/browse/gostub/gostub.go)

[rog, 2012-06-19 13:48:45]

looks good as a start.

the thing we really have to be careful about is the move away from atomicity
(for instance, AllMachines in the current state package returns an atomic
snapshot of all current machines, which doesn't look like it will be the case
here).

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode24
mstate/machine.go:24: mdoc := &machineDoc{}
On 2012/06/19 05:51:28, dfc wrote:
> var mdoc = new(machineDoc) ?

my preference would be
var doc machineDoc
and use &doc below.

[aram, 2012-06-19 13:58:35]

Unfortunately due to some bzr troubles, I had to submit a different CL:
https://codereview.appspot.com/6295103

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode20
mstate/machine.go:20: }
On 2012/06/19 05:51:28, dfc wrote:
> can we have a String() method as well please.

Sure, we will have everything, but in small bits at a time. This CL is already
too large. I'd rather do String in a subsequent CL.

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode24
mstate/machine.go:24: mdoc := &machineDoc{}
On 2012/06/19 05:51:28, dfc wrote:
> var mdoc = new(machineDoc) ?

I don't know, personally I prefer new(T), but the existing code uses &T{} almost
exclusively. Maybe it's time to decide on a convention?

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go
File mstate/mgo_test.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode18
mstate/mgo_test.go:18: const mgoport = "27017" // 50017
On 2012/06/19 05:51:28, dfc wrote:
> const (
> ...
> )

Done.

[dave_cheney.net, 2012-06-19 14:00:40]

Yup, proposing from your main branch uses up your one bullet. 

On 19/06/2012, at 23:58, aram@mgk.ro wrote:

> Unfortunately due to some bzr troubles, I had to submit a different CL:
> https://codereview.appspot.com/6295103
> 
> 
> https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
> File mstate/machine.go (right):
> 
> https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode20
> mstate/machine.go:20: }
> On 2012/06/19 05:51:28, dfc wrote:
>> can we have a String() method as well please.
> 
> Sure, we will have everything, but in small bits at a time. This CL is
> already too large. I'd rather do String in a subsequent CL.
> 
> https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode24
> mstate/machine.go:24: mdoc := &machineDoc{}
> On 2012/06/19 05:51:28, dfc wrote:
>> var mdoc = new(machineDoc) ?
> 
> I don't know, personally I prefer new(T), but the existing code uses
> &T{} almost exclusively. Maybe it's time to decide on a convention?
> 
> https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go
> File mstate/mgo_test.go (right):
> 
> https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode18
> mstate/mgo_test.go:18: const mgoport = "27017" // 50017
> On 2012/06/19 05:51:28, dfc wrote:
>> const (
>> ...
>> )
> 
> Done.
> 
> https://codereview.appspot.com/6304099/

[niemeyer, 2012-06-20 13:36:34]

Superb kick off. Thanks Aram!

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode18
mstate/machine.go:18: func (m *Machine) Id() string {
This has to be changed after the points made too.

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode23
mstate/machine.go:23: func (m *Machine) InstanceId() (iid string, err error) {
s/iid/id/

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode24
mstate/machine.go:24: mdoc := &machineDoc{}
On 2012/06/19 13:58:35, aram wrote:
> On 2012/06/19 05:51:28, dfc wrote:
> > var mdoc = new(machineDoc) ?
> 
> I don't know, personally I prefer new(T), but the existing code uses &T{}
almost
> exclusively. Maybe it's time to decide on a convention?

We've discussed this convention before, and sticked to &T{} indeed.

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode27
mstate/machine.go:27: return
Aram, we've had some large debate about errors in the state package, and even if
they are not perfect we're trying to make them consistently better.

Please have a look at the existing errors, and let's try to have something
equivalent here, including equivalent messages (the messages there were properly
reviewed recently).

The other methods should be reviewed in the same sense too.

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode29
mstate/machine.go:29: iid = mdoc.InstanceId
As discussed, this is overusing named results. The straightforward looks great
here:

return mdoc.InstanceId, nil

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode35
mstate/machine.go:35: err = m.st.machines.Update(bson.D{{"_id", m.id}},
bson.D{{"instanceid", id}})
Ditto, except this will need proper error messaging, which may make the named
result appropriate.

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go
File mstate/mgo_test.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode46
mstate/mgo_test.go:46: err := DropAll(mgoaddr)
On 2012/06/19 05:51:28, dfc wrote:
> if err := ...; err != nil {
>    c.Fatalf("mongo dun goofed:%v", err)
> }

The panic is fine here. This is not part of what is being tested. This is a
harness that should always work for the actual test to be attempted at all.

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode54
mstate/mgo_test.go:54: panic(err)
On 2012/06/19 05:51:28, dfc wrote:
> ditto

Same.

https://codereview.appspot.com/6304099/diff/3001/mstate/mgo_test.go#newcode69
mstate/mgo_test.go:69: time.Sleep(5e8)
On 2012/06/19 05:51:28, dfc wrote:
> Please use the time constants here, time is already imported. You can also
> reused environs.Attempt. 

That'd be more clear indeed.

Aram, please copy this over to the store package once people are happy with the
result, so we have both consistent.

https://codereview.appspot.com/6304099/diff/3001/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/open.go#newcode9
mstate/open.go:9: log.Printf("state: opening state; MongoDB servers: %q",
servers)
"opening state with servers: %q"

https://codereview.appspot.com/6304099/diff/3001/mstate/open.go#newcode16
mstate/open.go:16: session.Close()
There's no reason to clone a session and close then close it immediately. We can
just use the session itself by dropping the previous two lines above.

https://codereview.appspot.com/6304099/diff/3001/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/state.go#newcode20
mstate/state.go:20: id := bson.NewObjectId()
Please see below.

https://codereview.appspot.com/6304099/diff/3001/mstate/state.go#newcode30
mstate/state.go:30: func (s *State) RemoveMachine(id string) (err error) {
It's critical that the API remains the same. The machine id is an int.

Hmmm.. how can the original tests be passing with this?

Please double check that other APIs are also matching.

https://codereview.appspot.com/6304099/diff/3001/mstate/state.go#newcode48
mstate/state.go:48: func (s *State) Machine(id string) (m *Machine, err error) {
Method must be reviewed after the above points.

[rog, 2012-06-20 14:51:18]

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode24
mstate/machine.go:24: mdoc := &machineDoc{}
On 2012/06/20 13:36:34, niemeyer wrote:
> On 2012/06/19 13:58:35, aram wrote:
> > On 2012/06/19 05:51:28, dfc wrote:
> > > var mdoc = new(machineDoc) ?
> > 
> > I don't know, personally I prefer new(T), but the existing code uses &T{}
> almost
> > exclusively. Maybe it's time to decide on a convention?
> 
> We've discussed this convention before, and sticked to &T{} indeed.

we often use the

var buf bytes.Buffer
foo(&buf)

convention too, FWIW.

[aram, 2012-06-20 16:47:51]

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode23
mstate/machine.go:23: func (m *Machine) InstanceId() (iid string, err error) {
On 2012/06/20 13:36:34, niemeyer wrote:
> s/iid/id/

Removed named parameters altogether.

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode29
mstate/machine.go:29: iid = mdoc.InstanceId
On 2012/06/20 13:36:34, niemeyer wrote:
> As discussed, this is overusing named results. The straightforward looks great
> here:
> 
> return mdoc.InstanceId, nil

Done.

https://codereview.appspot.com/6304099/diff/3001/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/open.go#newcode9
mstate/open.go:9: log.Printf("state: opening state; MongoDB servers: %q",
servers)
On 2012/06/20 13:36:34, niemeyer wrote:
> "opening state with servers: %q"

Done.

https://codereview.appspot.com/6304099/diff/3001/mstate/open.go#newcode16
mstate/open.go:16: session.Close()
On 2012/06/20 13:36:34, niemeyer wrote:
> There's no reason to clone a session and close then close it immediately. We
can
> just use the session itself by dropping the previous two lines above.

Done.

https://codereview.appspot.com/6304099/diff/3001/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/state.go#newcode30
mstate/state.go:30: func (s *State) RemoveMachine(id string) (err error) {
On 2012/06/20 13:36:34, niemeyer wrote:
> It's critical that the API remains the same. The machine id is an int.
> 
> Hmmm.. how can the original tests be passing with this?
> 
> Please double check that other APIs are also matching.

Done. As per your suggestion ids are allocated from a sequence collection, where
the _id is the collection name.

[aram, 2012-06-20 16:48:40]

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/3001/mstate/machine.go#newcode27
mstate/machine.go:27: return
On 2012/06/20 13:36:34, niemeyer wrote:
> Aram, we've had some large debate about errors in the state package, and even
if
> they are not perfect we're trying to make them consistently better.
> 
> Please have a look at the existing errors, and let's try to have something
> equivalent here, including equivalent messages (the messages there were
properly
> reviewed recently).
> 
> The other methods should be reviewed in the same sense too.

Done.

[aram, 2012-06-20 16:49:55]

Please take a look.

[aram, 2012-06-20 18:05:21]

Please take a look.

[aram, 2012-06-20 18:18:33]

Please take a look.

[niemeyer, 2012-06-20 19:31:55]

This is looking great. Probably the last pass.

https://codereview.appspot.com/6304099/diff/9002/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/machine.go#newcode38
mstate/machine.go:38: err := m.st.machines.Update(bson.D{{"_id", m.id}},
bson.D{{"instanceid", id}})
This needs to use the $set syntax, otherwise it replaces the whole doc.

https://codereview.appspot.com/6304099/diff/9002/mstate/machine.go#newcode45
mstate/machine.go:45: // String returns a unique description of this machine
s/$/./

https://codereview.appspot.com/6304099/diff/9002/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/open.go#newcode9
mstate/open.go:9: log.Printf("state: opening state with servers: %q", servers)
s/state:// (we're dropping those prefixes)

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go
File mstate/sequence.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode10
mstate/sequence.go:10: Collection string `bson:"_id"`
s/Collection/Name/

This is a generic sequence mechanism. It may not be associated with a
collection.

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode16
mstate/sequence.go:16: inc := mgo.Change{Update: bson.M{"$inc":
bson.M{"counter": 1}}, Upsert: true}
Would you mind to break that line down, just for slightly improved readability:

inc := mgo.Change{
    Update: ...
    Upsert: ...
}

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode20
mstate/sequence.go:20: return -1, fmt.Errorf("can't increment %s sequence
number: %v", name, err)
s/%s/%q/

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode22
mstate/sequence.go:22: return result.Counter, err
s/err/nil/

https://codereview.appspot.com/6304099/diff/9002/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/state.go#newcode47
mstate/state.go:47: return
fmt.Errorf(<sensible message>, err)

https://codereview.appspot.com/6304099/diff/9002/mstate/state.go#newcode59
mstate/state.go:59: if err != nil {
if err == mgo.NotFound {

We'll need another branch for actual errors.

[aram, 2012-06-21 08:28:41]

https://codereview.appspot.com/6304099/diff/9002/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/machine.go#newcode38
mstate/machine.go:38: err := m.st.machines.Update(bson.D{{"_id", m.id}},
bson.D{{"instanceid", id}})
On 2012/06/20 19:31:55, niemeyer wrote:
> This needs to use the $set syntax, otherwise it replaces the whole doc.

Good catch, done.

https://codereview.appspot.com/6304099/diff/9002/mstate/machine.go#newcode45
mstate/machine.go:45: // String returns a unique description of this machine
On 2012/06/20 19:31:55, niemeyer wrote:
> s/$/./

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/open.go#newcode9
mstate/open.go:9: log.Printf("state: opening state with servers: %q", servers)
On 2012/06/20 19:31:55, niemeyer wrote:
> s/state:// (we're dropping those prefixes)

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go
File mstate/sequence.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode10
mstate/sequence.go:10: Collection string `bson:"_id"`
On 2012/06/20 19:31:55, niemeyer wrote:
> s/Collection/Name/
> 
> This is a generic sequence mechanism. It may not be associated with a
> collection.

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode16
mstate/sequence.go:16: inc := mgo.Change{Update: bson.M{"$inc":
bson.M{"counter": 1}}, Upsert: true}
On 2012/06/20 19:31:55, niemeyer wrote:
> Would you mind to break that line down, just for slightly improved
readability:
> 
> inc := mgo.Change{
>     Update: ...
>     Upsert: ...
> }

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode20
mstate/sequence.go:20: return -1, fmt.Errorf("can't increment %s sequence
number: %v", name, err)
On 2012/06/20 19:31:55, niemeyer wrote:
> s/%s/%q/

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/sequence.go#newcode22
mstate/sequence.go:22: return result.Counter, err
On 2012/06/20 19:31:55, niemeyer wrote:
> s/err/nil/

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6304099/diff/9002/mstate/state.go#newcode47
mstate/state.go:47: return
On 2012/06/20 19:31:55, niemeyer wrote:
> fmt.Errorf(<sensible message>, err)

Done.

https://codereview.appspot.com/6304099/diff/9002/mstate/state.go#newcode59
mstate/state.go:59: if err != nil {
On 2012/06/20 19:31:55, niemeyer wrote:
> if err == mgo.NotFound {
> 
> We'll need another branch for actual errors.

Done.

[aram, 2012-06-21 08:29:07]

Please take a look.

[niemeyer, 2012-06-21 16:21:44]

LGTM! Just a trivial before submitting please:

https://codereview.appspot.com/6304099/diff/15001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6304099/diff/15001/mstate/machine.go#newcode38
mstate/machine.go:38: change := bson.M{"$set": bson.D{{"instanceid", id}}}
Let's please use bson.D on both. It's significantly cheaper than creating a map,
and looks only slightly worse due to the extra parenthesis.

[aram, 2012-06-21 16:24:49]

Please take a look.

[aram, 2012-06-21 16:26:56]

*** Submitted:

mstate: state package replacement using mgo.

This is the first step towards replacing Zookeeper with MongoDB.
Mstate will slowly reimplement the state package while preserving the
original state API.

This change implements preliminary support for Machines.

R=dfc, rog, niemeyer
CC=
https://codereview.appspot.com/6304099