Complete Service without watches and Charm.

Complete the Service state without watches and Charm (only
a mock as a first step). State also can add and remove
services. Migration of YAMLState to new type ConfigNode
for key/value data in ZooKeeper nodes.

https://code.launchpad.net/~themue/juju/go-state-service-wo-watches/+merge/89012

(do not edit description out of merge proposal)

[TheMue, 2012-01-18 11:30:55]

Please take a look.

[niemeyer, 2012-01-18 13:29:18]

Looking good. Besides the comments that follow, there are two top level problems
we have to fix, and it'd be good to get it done in this branch:

1) We need complete tests for topology ensuring the interface expected by the
other files works. See the python logic for reference.

2) We need complete tests for ConfigNode as well. Also see the python logic for
reference.

https://codereview.appspot.com/5554047/diff/1/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5554047/diff/1/state/charm.go#newcode20
state/charm.go:20: return c.url.String()
Let's please change this to:

// URL returns the URL that identifies the charm.
func (c *Charm) URL() *charm.URL {
    clone := *c.url
    return &clone
}

https://codereview.appspot.com/5554047/diff/1/state/charm.go#newcode29
state/charm.go:29: }
This looks very strange. It's a public API we don't want, and it's doing nothing
besides *ignoring the error* (!). Please don't ever ignore errors without a
proper justification in a comment above it explaining why the error is being
ignored.

In this case, though, this should just be removed.

https://codereview.appspot.com/5554047/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode39
state/service.go:39: func (s *Service) SetCharmId(charmId string) error {
Let's change this to:

// SetCharmURL changes the charm URL for the service.
func SetCharmURL(url *charm.URL) error

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode197
state/service.go:197: // ZooKeeper.
This whole package is about ZooKeeper, so we don't have to bring it up in
comments. Also, when possible it'd be nice to be more clear about what the
concept means. E.g.:

// IsExposed returns whether this service is exposed.
// The explicitly open ports (with open-port) for exposed
// services may be accessed from machines outside of the
// local deployment network. See SetExposed and ClearExposed.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode206
state/service.go:206: // SetExposed marks the service as exposed in ZooKeeper.
// SetExposed marks the service as exposed.
// See ClearExposed and IsExposed.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode208
state/service.go:208: if _, err := s.zk.Create(s.zkExposedPath(), "", 0,
zookeeper.WorldACL(zookeeper.PERM_ALL)); err != nil {
In general, we try to put the exit path first. E.g.:

err := s.zk.Create(...)
if err == nil {
    return nil
}
... rest of the logic ...

This makes it easier to read and avoids the depth right-wise. That said, see the
next comment.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode211
state/service.go:211: if zkErr == -110 {
This is more complex than it has to be, and the use of numbers like this makes
it hard to read. I believe the whole function can actually be something like:

err := s.zk.Create(...)
if err != nil && err != zookeeper.ZNODEEXISTS {
    return err
}
return nil

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode220
state/service.go:220: // ClearExposed removes the exposed flag of the service in
ZooKeeper.
// ClearExposed removes the exposed flag from the service.
// See SetExposed and IsExposed.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode225
state/service.go:225: if zkErr == -101 {
Please change the implementation of this function following the previous ideas.

https://codereview.appspot.com/5554047/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5554047/diff/1/state/state.go#newcode21
state/state.go:21: oldTopology *topology
Why is this being stored?

https://codereview.appspot.com/5554047/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode68
state/state_test.go:68: // Connect the server.
init() and done() as they are now (without fake data) can become SetUpTest and
TearDownTest, so that you don't have to call them on every single test.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode101
state/state_test.go:101: c.Assert(err, IsNil)
You can also move state.Open onto s so that it's available as s.st and you don't
have to re-open it on all tests.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode105
state/state_test.go:105: wordpressSvc, err := st.AddService("wordpress", charm)
s/wordpressSvc/wordpress/

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode108
state/state_test.go:108: mySqlSvc, err := st.AddService("mysql", charm)
s/mySqlSvc/mysql/

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode160
state/state_test.go:160: // Check that retrieving a non-existent service fails
nicely.
This is new, but otherwise the previous logic is already in TestAddService.
Please rename it to TestReadNonExistentService and drop the duplicated logic.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode165
state/state_test.go:165: func (s StateSuite) TestWriteService(c *C) {
This test is huge. Please break it down into independent tests with separate
concerns.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode283
state/state_test.go:283: // Create test scenario (yet manually).
I don't know what you intend to say by (yet manually). Please expand the comment
for clarity.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode341
state/state_test.go:341: // Create test scenario (yet manually).
Ditto.

https://codereview.appspot.com/5554047/diff/1/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5554047/diff/1/state/topology.go#newcode55
state/topology.go:55: if zkErr == -101 {
Please fix this according to the previous comments.

https://codereview.appspot.com/5554047/diff/1/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode19
state/util.go:19: itemAdded = iota
s/iota/iota+1/

It's easy to make mistakes using zero constants.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode53
state/util.go:53: // and the pending writes.
Please replace the whole comment by this:

// A ConfigNode represents the data of a ZooKeeper node
// containing YAML-based settings. It manages changes to
// the data as a delta in memory, and merges them back
// onto the node when explicitly requested.

We'll explain the behavior of methods on their respective documentation.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode62
state/util.go:62: // a new service.
s/, .*/./

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode77
state/util.go:77: // readConfigNode reads the current ZooKeeper config data for
a node.
// readConfigNode returns the ConfigNode for path.
// If required is true, an error will be returned if
// the path doesn't exist.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode89
state/util.go:89: if zkErr == -101 && required {
Please adjust this according to the other ones.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode95
state/util.go:95: if err = goyaml.Unmarshal([]byte(yaml), c.cache); err != nil {
Hmmm.. we should probably change gozk to return []byte from Get at some point.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode103
state/util.go:103: // merged, the write buffer resetted.
// Write writes changes made to c back onto its node.
// Changes are written as a delta applied on top of the
// latest version of the node, to prevent overwriting
// unrelated changes made to the node since it was last read.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode108
state/util.go:108: // Changes records the changes done in applyChanges.
The original comment was more clear:

// changes is used by applyChanges to return the changes to
// this scope.

Please put it right above the changes declaration too.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode109
state/util.go:109: missing := new(bool)
This is a weird construct, so we need a comment:

// nil is a possible value for a key, so we use missing as
// a marker to simplify the algorithm below.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode115
state/util.go:115: changes = []*ItemChange{}
We don't need to reallocate another slice here:

changes = changes[:0]

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode131
state/util.go:131: if oldValue != newValue {
Declare this here:

var change *ItemChange

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode135
state/util.go:135: logChange(itemModified, key, oldValue, newValue)
Use:

change = &ItemChange{itemModified, key, oldValue, newValue}

logChange takes exactly the same args, so we're not winning much by using a
closure and losing in clarity.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode137
state/util.go:137: logChange(itemAdded, key, nil, newValue)
Ditto.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode141
state/util.go:141: logChange(itemDeleted, key, oldValue, nil)
Ditto.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode142
state/util.go:142: }
changes = append(changes, change)

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode157
state/util.go:157: // Keys returns the keys of a config node as slice.
// Keys returns the current keys.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode166
state/util.go:166: // Get retrieves a value by its key and returns nil if it
doesn't exist.
Let's make Get return a bool that informs whether the key was found or not. This
enables us to drop GetDefault.

// Get returns the value of key and whether it was found.
func (c *ConfigNode) Get(key string) (value interface{}, found bool) {

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode173
state/util.go:173: func (c *ConfigNode) GetDefault(key string, dflt interface{})
interface{} {
Please drop it. Get can handle the use case with the suggested change.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode181
state/util.go:181: // returns the old value.
Let's drop the result. People can use Get if they wish:

// Set sets key to value.
func (c *ConfigNode) Set(key string, value interface{}) {

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode189
state/util.go:189: // return the value.
Similarly:

// Delete removes key.
func (c *ConfigNode) Delete(key string) {

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode221
state/util.go:221: // cacheKeys creates a slice of the keys of multiple caches.
// cacheKeys returns the keys of all caches as a key=>true map.

[rog, 2012-01-18 14:44:07]

a few minor extra comments

https://codereview.appspot.com/5554047/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode198
state/service.go:198: func (s *Service) IsExposed() (bool, error) {
"Exposed" might be better than IsExposed.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode207
state/service.go:207: func (s *Service) SetExposed() error {
perhaps SetExposed(exposed bool) error
might be better.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode235
state/service.go:235: // changes have been done the nodes Write() method has to
be
this reads awkwardly to me.

perhaps something like this instead?

"Config returns a configuration node for the service. This may be changed and
its Write method used to make the changes persistent."

https://codereview.appspot.com/5554047/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5554047/diff/1/state/state.go#newcode42
state/state.go:42: path, err := s.zk.Create("/services/service-", string(yaml),
zookeeper.SEQUENCE, zookeeper.WorldACL(zookeeper.PERM_ALL))
i think it might be useful to define a global variable to represent global
permissions,
e.g.
var permAll = zookeeper.WorldACL(zookeeper.PERM_ALL)
and use it throughout.

https://codereview.appspot.com/5554047/diff/1/state/state.go#newcode44
state/state.go:44: return nil, err
i think it's good practice to add some info to errors like this, otherwise it
doesn't have any context.
e.g. return fmt.Errorf("cannot create new zk service node: %v", err)

[niemeyer, 2012-01-18 15:47:02]

https://codereview.appspot.com/5554047/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode198
state/service.go:198: func (s *Service) IsExposed() (bool, error) {
On 2012/01/18 14:44:07, rog wrote:
> "Exposed" might be better than IsExposed.

+1

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode207
state/service.go:207: func (s *Service) SetExposed() error {
On 2012/01/18 14:44:07, rog wrote:
> perhaps SetExposed(exposed bool) error
> might be better.

Frank had a point about this, but I can't remember. I actually prefer
SetExposed(bool) too, using the same logic as Set+Clean. Frank?

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode235
state/service.go:235: // changes have been done the nodes Write() method has to
be
On 2012/01/18 14:44:07, rog wrote:
> this reads awkwardly to me.
> 
> perhaps something like this instead?
> 
> "Config returns a configuration node for the service. This may be changed and
> its Write method used to make the changes persistent."

This is awkward too, and not clear. "This" is ambiguous.

Frank, please reduce your sentence to:

// Config returns the configuration node for the service.

No need to explain further. ConfigNode's docs may be read for what to do with
it.

[rog, 2012-01-18 16:14:13]

On 18 January 2012 15:47,  <n13m3y3r@gmail.com> wrote:
>
> https://codereview.appspot.com/5554047/diff/1/state/service.go
> File state/service.go (right):
>
> https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode198
> state/service.go:198: func (s *Service) IsExposed() (bool, error) {
> On 2012/01/18 14:44:07, rog wrote:
>>
>> "Exposed" might be better than IsExposed.
>
>
> +1
>
>
> https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode207
> state/service.go:207: func (s *Service) SetExposed() error {
> On 2012/01/18 14:44:07, rog wrote:
>>
>> perhaps SetExposed(exposed bool) error
>> might be better.
>
>
> Frank had a point about this, but I can't remember. I actually prefer
> SetExposed(bool) too, using the same logic as Set+Clean. Frank?

or even, as just discussed in IRC, simply Expose.

as in:

service.Expose(otherService.Exposed())

i think i quite like that.

> https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode235
> state/service.go:235: // changes have been done the nodes Write() method
> has to be
> On 2012/01/18 14:44:07, rog wrote:
>>
>> this reads awkwardly to me.
>
>
>> perhaps something like this instead?
>
>
>> "Config returns a configuration node for the service. This may be
>
> changed and
>>
>> its Write method used to make the changes persistent."
>
>
> This is awkward too, and not clear. "This" is ambiguous.
>
> Frank, please reduce your sentence to:
>
> // Config returns the configuration node for the service.
>
> No need to explain further. ConfigNode's docs may be read for what to do
> with it.

+1

[TheMue, 2012-01-28 19:53:02]

Please take a look.

[TheMue, 2012-01-30 14:02:16]

Please take a look.

https://codereview.appspot.com/5554047/diff/1/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5554047/diff/1/state/charm.go#newcode20
state/charm.go:20: return c.url.String()
On 2012/01/18 13:29:18, niemeyer wrote:
> Let's please change this to:
> 
> // URL returns the URL that identifies the charm.
> func (c *Charm) URL() *charm.URL {
>     clone := *c.url
>     return &clone
> }

Done.

https://codereview.appspot.com/5554047/diff/1/state/charm.go#newcode29
state/charm.go:29: }
On 2012/01/18 13:29:18, niemeyer wrote:
> This looks very strange. It's a public API we don't want, and it's doing
nothing
> besides *ignoring the error* (!). Please don't ever ignore errors without a
> proper justification in a comment above it explaining why the error is being
> ignored.
> 
> In this case, though, this should just be removed.

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode39
state/service.go:39: func (s *Service) SetCharmId(charmId string) error {
On 2012/01/18 13:29:18, niemeyer wrote:
> Let's change this to:
> 
> // SetCharmURL changes the charm URL for the service.
> func SetCharmURL(url *charm.URL) error

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode197
state/service.go:197: // ZooKeeper.
On 2012/01/18 13:29:18, niemeyer wrote:
> This whole package is about ZooKeeper, so we don't have to bring it up in
> comments. Also, when possible it'd be nice to be more clear about what the
> concept means. E.g.:
> 
> // IsExposed returns whether this service is exposed.
> // The explicitly open ports (with open-port) for exposed
> // services may be accessed from machines outside of the
> // local deployment network. See SetExposed and ClearExposed.

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode198
state/service.go:198: func (s *Service) IsExposed() (bool, error) {
On 2012/01/18 15:47:02, niemeyer wrote:
> On 2012/01/18 14:44:07, rog wrote:
> > "Exposed" might be better than IsExposed.
> 
> +1

After IRC discussion I'm keeping IsExposed() here.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode206
state/service.go:206: // SetExposed marks the service as exposed in ZooKeeper.
On 2012/01/18 13:29:18, niemeyer wrote:
> // SetExposed marks the service as exposed.
> // See ClearExposed and IsExposed.

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode207
state/service.go:207: func (s *Service) SetExposed() error {
On 2012/01/18 15:47:02, niemeyer wrote:
> On 2012/01/18 14:44:07, rog wrote:
> > perhaps SetExposed(exposed bool) error
> > might be better.
> 
> Frank had a point about this, but I can't remember. I actually prefer
> SetExposed(bool) too, using the same logic as Set+Clean. Frank?

See IRC.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode208
state/service.go:208: if _, err := s.zk.Create(s.zkExposedPath(), "", 0,
zookeeper.WorldACL(zookeeper.PERM_ALL)); err != nil {
On 2012/01/18 13:29:18, niemeyer wrote:
> In general, we try to put the exit path first. E.g.:
> 
> err := s.zk.Create(...)
> if err == nil {
>     return nil
> }
> ... rest of the logic ...
> 
> This makes it easier to read and avoids the depth right-wise. That said, see
the
> next comment.

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode211
state/service.go:211: if zkErr == -110 {
On 2012/01/18 13:29:18, niemeyer wrote:
> This is more complex than it has to be, and the use of numbers like this makes
> it hard to read. I believe the whole function can actually be something like:
> 
> err := s.zk.Create(...)
> if err != nil && err != zookeeper.ZNODEEXISTS {
>     return err
> }
> return nil

Done. Yeah, looks definitely better. Sadly GoPkgDoc don't shows the Z... errors,
only the type, while godoc itself does it.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode220
state/service.go:220: // ClearExposed removes the exposed flag of the service in
ZooKeeper.
On 2012/01/18 13:29:18, niemeyer wrote:
> // ClearExposed removes the exposed flag from the service.
> // See SetExposed and IsExposed.

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode225
state/service.go:225: if zkErr == -101 {
On 2012/01/18 13:29:18, niemeyer wrote:
> Please change the implementation of this function following the previous
ideas.

Done.

https://codereview.appspot.com/5554047/diff/1/state/service.go#newcode235
state/service.go:235: // changes have been done the nodes Write() method has to
be
On 2012/01/18 15:47:02, niemeyer wrote:
> On 2012/01/18 14:44:07, rog wrote:
> > this reads awkwardly to me.
> > 
> > perhaps something like this instead?
> > 
> > "Config returns a configuration node for the service. This may be changed
and
> > its Write method used to make the changes persistent."
> 
> This is awkward too, and not clear. "This" is ambiguous.
> 
> Frank, please reduce your sentence to:
> 
> // Config returns the configuration node for the service.
> 
> No need to explain further. ConfigNode's docs may be read for what to do with
> it.

Done.

https://codereview.appspot.com/5554047/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5554047/diff/1/state/state.go#newcode21
state/state.go:21: oldTopology *topology
On 2012/01/18 13:29:18, niemeyer wrote:
> Why is this being stored?

Done.

https://codereview.appspot.com/5554047/diff/1/state/state.go#newcode42
state/state.go:42: path, err := s.zk.Create("/services/service-", string(yaml),
zookeeper.SEQUENCE, zookeeper.WorldACL(zookeeper.PERM_ALL))
On 2012/01/18 14:44:07, rog wrote:
> i think it might be useful to define a global variable to represent global
> permissions,
> e.g.
> var permAll = zookeeper.WorldACL(zookeeper.PERM_ALL)
> and use it throughout.

Done. Good idea.

https://codereview.appspot.com/5554047/diff/1/state/state.go#newcode44
state/state.go:44: return nil, err
On 2012/01/18 14:44:07, rog wrote:
> i think it's good practice to add some info to errors like this, otherwise it
> doesn't have any context.
> e.g. return fmt.Errorf("cannot create new zk service node: %v", err)

Thought about it. I prefer that each error returning func is responsible for the
right information. Otherwise wrapping it could lead to stacks similar to Java
stack traces:

"can't deploy charm: can't create service state: can't create path: can't write
file xyz"

https://codereview.appspot.com/5554047/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode68
state/state_test.go:68: // Connect the server.
On 2012/01/18 13:29:18, niemeyer wrote:
> init() and done() as they are now (without fake data) can become SetUpTest and
> TearDownTest, so that you don't have to call them on every single test. 

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode101
state/state_test.go:101: c.Assert(err, IsNil)
On 2012/01/18 13:29:18, niemeyer wrote:
> You can also move state.Open onto s so that it's available as s.st and you
don't
> have to re-open it on all tests.

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode105
state/state_test.go:105: wordpressSvc, err := st.AddService("wordpress", charm)
On 2012/01/18 13:29:18, niemeyer wrote:
> s/wordpressSvc/wordpress/

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode108
state/state_test.go:108: mySqlSvc, err := st.AddService("mysql", charm)
On 2012/01/18 13:29:18, niemeyer wrote:
> s/mySqlSvc/mysql/

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode160
state/state_test.go:160: // Check that retrieving a non-existent service fails
nicely.
On 2012/01/18 13:29:18, niemeyer wrote:
> This is new, but otherwise the previous logic is already in TestAddService.
> Please rename it to TestReadNonExistentService and drop the duplicated logic.

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode165
state/state_test.go:165: func (s StateSuite) TestWriteService(c *C) {
On 2012/01/18 13:29:18, niemeyer wrote:
> This test is huge. Please break it down into independent tests with separate
> concerns.

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode283
state/state_test.go:283: // Create test scenario (yet manually).
On 2012/01/18 13:29:18, niemeyer wrote:
> I don't know what you intend to say by (yet manually). Please expand the
comment
> for clarity.

Done.

https://codereview.appspot.com/5554047/diff/1/state/state_test.go#newcode341
state/state_test.go:341: // Create test scenario (yet manually).
On 2012/01/18 13:29:18, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/5554047/diff/1/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5554047/diff/1/state/topology.go#newcode55
state/topology.go:55: if zkErr == -101 {
On 2012/01/18 13:29:18, niemeyer wrote:
> Please fix this according to the previous comments.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode19
state/util.go:19: itemAdded = iota
On 2012/01/18 13:29:18, niemeyer wrote:
> s/iota/iota+1/
> 
> It's easy to make mistakes using zero constants.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode53
state/util.go:53: // and the pending writes.
On 2012/01/18 13:29:18, niemeyer wrote:
> Please replace the whole comment by this:
> 
> // A ConfigNode represents the data of a ZooKeeper node
> // containing YAML-based settings. It manages changes to
> // the data as a delta in memory, and merges them back
> // onto the node when explicitly requested.
> 
> We'll explain the behavior of methods on their respective documentation.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode62
state/util.go:62: // a new service.
On 2012/01/18 13:29:18, niemeyer wrote:
> s/, .*/./

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode77
state/util.go:77: // readConfigNode reads the current ZooKeeper config data for
a node.
On 2012/01/18 13:29:18, niemeyer wrote:
> // readConfigNode returns the ConfigNode for path.
> // If required is true, an error will be returned if
> // the path doesn't exist.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode89
state/util.go:89: if zkErr == -101 && required {
On 2012/01/18 13:29:18, niemeyer wrote:
> Please adjust this according to the other ones.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode95
state/util.go:95: if err = goyaml.Unmarshal([]byte(yaml), c.cache); err != nil {
On 2012/01/18 13:29:18, niemeyer wrote:
> Hmmm.. we should probably change gozk to return []byte from Get at some point.

It's annoying sometimes, but it's ok so. No urgent need.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode103
state/util.go:103: // merged, the write buffer resetted.
On 2012/01/18 13:29:18, niemeyer wrote:
> // Write writes changes made to c back onto its node.
> // Changes are written as a delta applied on top of the
> // latest version of the node, to prevent overwriting
> // unrelated changes made to the node since it was last read.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode108
state/util.go:108: // Changes records the changes done in applyChanges.
On 2012/01/18 13:29:18, niemeyer wrote:
> The original comment was more clear:
> 
> // changes is used by applyChanges to return the changes to
> // this scope.
> 
> Please put it right above the changes declaration too.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode109
state/util.go:109: missing := new(bool)
On 2012/01/18 13:29:18, niemeyer wrote:
> This is a weird construct, so we need a comment:
> 
> // nil is a possible value for a key, so we use missing as
> // a marker to simplify the algorithm below.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode115
state/util.go:115: changes = []*ItemChange{}
On 2012/01/18 13:29:18, niemeyer wrote:
> We don't need to reallocate another slice here:
> 
> changes = changes[:0]

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode131
state/util.go:131: if oldValue != newValue {
On 2012/01/18 13:29:18, niemeyer wrote:
> Declare this here:
> 
> var change *ItemChange

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode135
state/util.go:135: logChange(itemModified, key, oldValue, newValue)
On 2012/01/18 13:29:18, niemeyer wrote:
> Use:
> 
> change = &ItemChange{itemModified, key, oldValue, newValue}
> 
> logChange takes exactly the same args, so we're not winning much by using a
> closure and losing in clarity.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode137
state/util.go:137: logChange(itemAdded, key, nil, newValue)
On 2012/01/18 13:29:18, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode141
state/util.go:141: logChange(itemDeleted, key, oldValue, nil)
On 2012/01/18 13:29:18, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode142
state/util.go:142: }
On 2012/01/18 13:29:18, niemeyer wrote:
> changes = append(changes, change)

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode157
state/util.go:157: // Keys returns the keys of a config node as slice.
On 2012/01/18 13:29:18, niemeyer wrote:
> // Keys returns the current keys.

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode166
state/util.go:166: // Get retrieves a value by its key and returns nil if it
doesn't exist.
On 2012/01/18 13:29:18, niemeyer wrote:
> Let's make Get return a bool that informs whether the key was found or not.
This
> enables us to drop GetDefault.
> 
> // Get returns the value of key and whether it was found.
> func (c *ConfigNode) Get(key string) (value interface{}, found bool) {

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode173
state/util.go:173: func (c *ConfigNode) GetDefault(key string, dflt interface{})
interface{} {
On 2012/01/18 13:29:18, niemeyer wrote:
> Please drop it. Get can handle the use case with the suggested change.

Done. Has been intended to just be a convenience method.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode181
state/util.go:181: // returns the old value.
On 2012/01/18 13:29:18, niemeyer wrote:
> Let's drop the result. People can use Get if they wish:
> 
> // Set sets key to value.
> func (c *ConfigNode) Set(key string, value interface{}) {

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode189
state/util.go:189: // return the value.
On 2012/01/18 13:29:18, niemeyer wrote:
> Similarly:
> 
> // Delete removes key.
> func (c *ConfigNode) Delete(key string) {

Done.

https://codereview.appspot.com/5554047/diff/1/state/util.go#newcode221
state/util.go:221: // cacheKeys creates a slice of the keys of multiple caches.
On 2012/01/18 13:29:18, niemeyer wrote:
> // cacheKeys returns the keys of all caches as a key=>true map.

Done.

[niemeyer, 2012-01-30 14:39:53]

LGTM, assuming the following minor details are fixed.

Thanks for the great work, Frank.

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go#newco...
state/internal_test.go:95: c.Assert(s.t.HasService("s-0"), Equals, false)
This is pretty much the same as TestAddService. Just move the above line to
TestAddService before the service creation (it's the test missing there) and
drop this test.

https://codereview.appspot.com/5554047/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/service.go#newcode40
state/service.go:40: return url, nil
Replace the logic above by:

return charm.ParseURL(id.(string))

Also, make sure you update your tree and run tests. NewURL is gone for quite a
bit.

[fwereade, 2012-01-30 15:55:55]

Can't see any actual problems, so I made some nitpicky spelling/grammar comments
;).

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go#newco...
state/internal_test.go:74: // Check that adding services works correctly.
Out of interest: is there a specific reason not to write these comments as
doc-comments just before the function?

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go#newco...
state/internal_test.go:195: c.Assert(err, ErrorMatches, `unit "u-0" already in
use in servie "s-0"`)
s/servie/service/

(other instances ignored)

https://codereview.appspot.com/5554047/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/state.go#newcode43
state/state.go:43: // Create an empty chonfiguration node.
s/chonfiguration/configuration/

https://codereview.appspot.com/5554047/diff/6001/state/state.go#newcode62
state/state.go:62: // also remove all its units and breaks any of its existing
s/breaks/break/

https://codereview.appspot.com/5554047/diff/6001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/5554047/diff/6001/state/state_test.go#oldcode156
state/state_test.go:156: c.Assert(err, ErrorMatches, `"wordpress" is no valid
unit name`)
s/no/not a/

https://codereview.appspot.com/5554047/diff/6001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/topology.go#newcode70
state/topology.go:70: // Version returns the version of the topology.
I've been prioritizing code-reference correctness over mere linguistic
convention; is this wrong?

https://codereview.appspot.com/5554047/diff/6001/state/topology.go#newcode193
state/topology.go:193: // UnitKeyFromSequence returns the key of a unit by the
its service key
s/by the/based on/
?

[TheMue, 2012-01-30 19:34:41]

Please take a look.

[TheMue, 2012-01-30 19:34:59]

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go#newco...
state/internal_test.go:74: // Check that adding services works correctly.
On 2012/01/30 15:55:55, fwereade wrote:
> Out of interest: is there a specific reason not to write these comments as
> doc-comments just before the function?

Should be discussed in IRC, has been a review result of a former branch.

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go#newco...
state/internal_test.go:95: c.Assert(s.t.HasService("s-0"), Equals, false)
On 2012/01/30 14:39:53, niemeyer wrote:
> This is pretty much the same as TestAddService. Just move the above line to
> TestAddService before the service creation (it's the test missing there) and
> drop this test.

Done.

https://codereview.appspot.com/5554047/diff/6001/state/internal_test.go#newco...
state/internal_test.go:195: c.Assert(err, ErrorMatches, `unit "u-0" already in
use in servie "s-0"`)
On 2012/01/30 15:55:55, fwereade wrote:
> s/servie/service/
> 
> (other instances ignored)

Done.

https://codereview.appspot.com/5554047/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/service.go#newcode40
state/service.go:40: return url, nil
On 2012/01/30 14:39:53, niemeyer wrote:
> Replace the logic above by:
> 
> return charm.ParseURL(id.(string))
> 
> Also, make sure you update your tree and run tests. NewURL is gone for quite a
> bit.

Done.

https://codereview.appspot.com/5554047/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/state.go#newcode43
state/state.go:43: // Create an empty chonfiguration node.
On 2012/01/30 15:55:55, fwereade wrote:
> s/chonfiguration/configuration/

Done.

https://codereview.appspot.com/5554047/diff/6001/state/state.go#newcode62
state/state.go:62: // also remove all its units and breaks any of its existing
On 2012/01/30 15:55:55, fwereade wrote:
> s/breaks/break/

Done.

https://codereview.appspot.com/5554047/diff/6001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/5554047/diff/6001/state/state_test.go#oldcode156
state/state_test.go:156: c.Assert(err, ErrorMatches, `"wordpress" is no valid
unit name`)
On 2012/01/30 15:55:55, fwereade wrote:
> s/no/not a/

Done.

https://codereview.appspot.com/5554047/diff/6001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5554047/diff/6001/state/topology.go#newcode70
state/topology.go:70: // Version returns the version of the topology.
On 2012/01/30 15:55:55, fwereade wrote:
> I've been prioritizing code-reference correctness over mere linguistic
> convention; is this wrong?

Typo in method name, comment changed, but forgot method.

https://codereview.appspot.com/5554047/diff/6001/state/topology.go#newcode193
state/topology.go:193: // UnitKeyFromSequence returns the key of a unit by the
its service key
On 2012/01/30 15:55:55, fwereade wrote:
> s/by the/based on/
> ?

Done.

[niemeyer, 2012-01-30 19:47:31]

LGTM

[TheMue, 2012-01-30 19:50:35]

*** Submitted:

Complete Service without watches and Charm.

Complete the Service state without watches and Charm (only
a mock as a first step). State also can add and remove
services. Migration of YAMLState to new type ConfigNode
for key/value data in ZooKeeper nodes.

R=niemeyer, rog, fwereade
CC=
https://codereview.appspot.com/5554047