state: factor out allWatcher

state: factor out allWatcher

As suggested by dimiter.
The functionality is more general than just watching everything,
so names are changed accordingly.

There are no significant logic changes in this branch.

https://code.launchpad.net/~rogpeppe/juju-core/271-megawatcher-factor-out/+merge/157656

(do not edit description out of merge proposal)

[rog, 2013-04-08 13:48:47]

Please take a look.

[rog, 2013-04-08 13:52:01]

On 2013/04/08 13:48:47, rog wrote:
> Please take a look.

i also changed .lbox.check to use bash, because it's not valid sh.

[dimitern, 2013-04-08 15:43:12]

Much nicer, thanks! LGTM, with a lot of suggestions, mostly typos.

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go
File state/megabacking.go (right):

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go#newcode31
state/megabacking.go:31: // idOf returns the id of the given info.
put a NL before this comment please

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go#newcode127
state/megabacking.go:127: // changed updates the allWatcher's idea of the
current state
s/changed/Changed/

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go#newcode156
state/megabacking.go:156: // idForInfo returns the info id of the given entity
document.
s/idForInfo/IdForInfo/

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

https://codereview.appspot.com/8458044/diff/2001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:309: // Testmultiwatcher.StateWatcher tests
the integration of the state watcher
s/Testmultiwatcher./Test/

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
File state/multiwatcher/multiwatcher.go (right):

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:14: all *StoreManager
blank line after this

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:21: func NewWatcher(all *StoreManager)
*Watcher {
doc comment, since now it's exported

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:62: // StoreManager holds a shared record of
current state and replies to
"Store" manager? It's a bit confusing with the charm store perhaps.
StateObserver maybe?

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:85: // Backing is the interface required
can you reformat the comment so it's not so jaggy?

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:93: // getAll retrieves information about all
information
s/getAll/GetAll/

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:134: // newStoreManager returns a new
StoreManager that retrieves information
s/newStoreManager/NewStoreManager/

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:145: func (aw *StoreManager) Run() {
doc comment

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:155: func (aw *StoreManager) loop() error {
maybe replace the receivers of all methods to "sm" instead of "aw" ?

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:412: // as removed and deleted if nothing has
seen the entity..
s/entity../entity./

[rog, 2013-04-09 07:41:16]

Please take a look.

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go
File state/megabacking.go (right):

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go#newcode31
state/megabacking.go:31: // idOf returns the id of the given info.
On 2013/04/08 15:43:13, dimitern wrote:
> put a NL before this comment please

Done.

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go#newcode127
state/megabacking.go:127: // changed updates the allWatcher's idea of the
current state
On 2013/04/08 15:43:13, dimitern wrote:
> s/changed/Changed/

Done.

https://codereview.appspot.com/8458044/diff/2001/state/megabacking.go#newcode156
state/megabacking.go:156: // idForInfo returns the info id of the given entity
document.
On 2013/04/08 15:43:13, dimitern wrote:
> s/idForInfo/IdForInfo/

Done.

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

https://codereview.appspot.com/8458044/diff/2001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:309: // Testmultiwatcher.StateWatcher tests
the integration of the state watcher
On 2013/04/08 15:43:13, dimitern wrote:
> s/Testmultiwatcher./Test/

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
File state/multiwatcher/multiwatcher.go (right):

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:14: all *StoreManager
On 2013/04/08 15:43:13, dimitern wrote:
> blank line after this

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:21: func NewWatcher(all *StoreManager)
*Watcher {
On 2013/04/08 15:43:13, dimitern wrote:
> doc comment, since now it's exported

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:62: // StoreManager holds a shared record of
current state and replies to
On 2013/04/08 15:43:13, dimitern wrote:
> "Store" manager? It's a bit confusing with the charm store perhaps.
> StateObserver maybe?

well, it is "multiwatcher.StoreManager" to go along with "multiwatcher.Store".

i don't think StateObserver is right - it doesn't really observe the State and
Observer is a synonym for Watcher.

i remain open to other suggestions though.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:85: // Backing is the interface required
On 2013/04/08 15:43:13, dimitern wrote:
> can you reformat the comment so it's not so jaggy?

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:93: // getAll retrieves information about all
information
On 2013/04/08 15:43:13, dimitern wrote:
> s/getAll/GetAll/

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:134: // newStoreManager returns a new
StoreManager that retrieves information
On 2013/04/08 15:43:13, dimitern wrote:
> s/newStoreManager/NewStoreManager/

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:145: func (aw *StoreManager) Run() {
On 2013/04/08 15:43:13, dimitern wrote:
> doc comment

removed the method and integrated into NewStoreManager. it should not have been
visible.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:155: func (aw *StoreManager) loop() error {
On 2013/04/08 15:43:13, dimitern wrote:
> maybe replace the receivers of all methods to "sm" instead of "aw" ?

Done.

https://codereview.appspot.com/8458044/diff/2001/state/multiwatcher/multiwatc...
state/multiwatcher/multiwatcher.go:412: // as removed and deleted if nothing has
seen the entity..
On 2013/04/08 15:43:13, dimitern wrote:
> s/entity../entity./

Done.

[TheMue, 2013-04-09 08:46:35]

LGTM, only one small comment. Also thinking about if it is worth defining
exposed constants for the EntityTypes. Right now they aren't directly compared,
so it's not needed.

https://codereview.appspot.com/8458044/diff/22001/state/multiwatcher/multiwat...
File state/multiwatcher/multiwatcher.go (right):

https://codereview.appspot.com/8458044/diff/22001/state/multiwatcher/multiwat...
state/multiwatcher/multiwatcher.go:150: s := newStoreManagerNoRun(backing)
Nothing important, but you name the StorageManager sm in the method declaration.
I would prefer it here too.

[rog, 2013-04-09 09:45:18]

*** Submitted:

state: factor out allWatcher

As suggested by dimiter.
The functionality is more general than just watching everything,
so names are changed accordingly.

There are no significant logic changes in this branch.

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

https://codereview.appspot.com/8458044/diff/22001/state/multiwatcher/multiwat...
File state/multiwatcher/multiwatcher.go (right):

https://codereview.appspot.com/8458044/diff/22001/state/multiwatcher/multiwat...
state/multiwatcher/multiwatcher.go:150: s := newStoreManagerNoRun(backing)
On 2013/04/09 08:46:35, TheMue wrote:
> Nothing important, but you name the StorageManager sm in the method
declaration.
> I would prefer it here too.

Done.