state: implement allWatcherStateBacking

state: implement allWatcherStateBacking

This completes the initial implementation of
StateWatcher - in the next branch, we'll
change the API to use it.

https://code.launchpad.net/~rogpeppe/juju-core/245-allwatcher-state-backing/+merge/153900

(do not edit description out of merge proposal)

[rog, 2013-03-18 18:28:34]

Please take a look.

[dave_cheney.net, 2013-03-19 01:05:30]

Some initial comments.

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go#newcode355
state/megawatcher.go:355: c *mgo.Collection
please don't call this c. If it didn't have a name then it could be c.Find(...),
rather than c.c.Find(...)

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go#newcode417
state/megawatcher.go:417: log.Printf("making new slice ptr from %v",
c.infoSliceType)
debugging ?

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go#newcode419
state/megawatcher.go:419: err := c.c.Find(nil).All(infoSlicePtr)
if err := ... ; err != nil { ... }

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:19: func AddTestingCharm(c *C, st *State,
name string) *Charm {
Why isn't this method on state itself, ie

func (s *State) AddTestingCharm(c *C, name string) *Charm

[rog, 2013-03-19 07:31:59]

Please take a look.

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go#newcode355
state/megawatcher.go:355: c *mgo.Collection
On 2013/03/19 01:05:30, dfc wrote:
> please don't call this c. If it didn't have a name then it could be
c.Find(...),
> rather than c.c.Find(...)

Done.

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go#newcode417
state/megawatcher.go:417: log.Printf("making new slice ptr from %v",
c.infoSliceType)
On 2013/03/19 01:05:30, dfc wrote:
> debugging ?

Done.

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher.go#newcode419
state/megawatcher.go:419: err := c.c.Find(nil).All(infoSlicePtr)
On 2013/03/19 01:05:30, dfc wrote:
> if err := ... ; err != nil { ... }

Done.

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7650048/diff/2001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:19: func AddTestingCharm(c *C, st *State,
name string) *Charm {
On 2013/03/19 01:05:30, dfc wrote:
> Why isn't this method on state itself, ie
> 
> func (s *State) AddTestingCharm(c *C, name string) *Charm

*shrug* done.

[dimitern, 2013-03-19 08:50:15]

Nice, LGTM, just some trivials.

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go#newcode433
state/megawatcher.go:433: log.Printf("collections: %+v", b.collectionByName)
Debugf?

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:988: //TestStateWatcher tests the integration
of the state watcher
//Test -> // Test

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:989: // wiwith the state-based backing. Most
of the logic is tested elsewhere -
s/wiwith/with/

[TheMue, 2013-03-19 15:09:13]

LGTM

[rog, 2013-03-20 15:09:58]

*** Submitted:

state: implement allWatcherStateBacking

This completes the initial implementation of
StateWatcher - in the next branch, we'll
change the API to use it.

R=dfc, dimitern, TheMue
CC=
https://codereview.appspot.com/7650048

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go#newcode433
state/megawatcher.go:433: log.Printf("collections: %+v", b.collectionByName)
On 2013/03/19 08:50:15, dimitern wrote:
> Debugf?

nah, it's just a debugging remnant. deleted.

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:988: //TestStateWatcher tests the integration
of the state watcher
On 2013/03/19 08:50:15, dimitern wrote:
> //Test -> // Test

Done.

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:989: // wiwith the state-based backing. Most
of the logic is tested elsewhere -
On 2013/03/19 08:50:15, dimitern wrote:
> s/wiwith/with/

Done.

[dave_cheney.net, 2013-03-25 02:21:01]

Some more suggestions, I think we're all commenting on older version.

https://codereview.appspot.com/7650048/diff/8001/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/7650048/diff/8001/state/conn_test.go#newcode59
state/conn_test.go:59: return s.State.AddTestingCharm(c, name)
You may want to consider embedding the *state.State into ConnSuite, so this
forwarding method is not needed.

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go#newcode343
state/megawatcher.go:343: type allWatcherStateBacking struct {
suggestion: stateBackedAllWatcher ?

https://codereview.appspot.com/7650048/diff/8001/state/megawatcher.go#newcode354
state/megawatcher.go:354: type allWatcherStateCollection struct {
suggestion: allWatcherCollection; this doesn't have a state.State member.