Issue 5502043: Initial adding of the state model to the Go port of juju (Closed)
DescriptionAs a first step in adding the Go port the state can be opened and first
operations ond Service and Unit can be done. Also needed first topology
functionality is part of this branch.
https://code.launchpad.net/~themue/juju/go-initial-state-adding/+merge/86373
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : Changes to the initial adding of state to the Go port. #
Total comments: 7
Patch Set 3 : Simplified the state code. #
Total comments: 71
Patch Set 4 : Adopted last review. #Patch Set 5 : Changed the state_test to an own package and added methods for Units #
Total comments: 234
Patch Set 6 : Initial adding of the state model to the Go port of juju #
Total comments: 20
Patch Set 7 : Initial adding of the state model to the Go port of juju #Patch Set 8 : Initial adding of the state model to the Go port of juju #
MessagesTotal messages: 24
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5502043/diff/2001/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/2001/state/service.go#newcode31 state/service.go:31: unit, ok := s.Units[id] We discussed this in the meeting, but it probably got lost in the conversation. These objects do not hold state. They communicate with ZooKeeper to obtain their state. It's fine to have a single topology, but we don't want to have a whole tree in memory that we then have to update whenever we're going to use it. https://codereview.appspot.com/5502043/diff/2001/state/service.go#newcode34 state/service.go:34: unit.topology = s.topology Nothing here allocates a Unit, so this will certainly panic. Even if it worked, though, it would be bogus, because it's hacking a value that is potentially in use without any synchronization. And even _with_ synchronization, it'd be wrong to do it, since the code using this unit is not expecting it to change behind its back. https://codereview.appspot.com/5502043/diff/2001/state/service.go#newcode45 state/service.go:45: func (s *Service) sync(newSvc *Service) error { Please drop this logic, for the reasons above. Once more, let's please start with something closer to what we have in Python. https://codereview.appspot.com/5502043/diff/2001/state/service.go#newcode52 state/service.go:52: s.CharmId = newSvc.CharmId We can't have fields like this. How do we change a field? https://codereview.appspot.com/5502043/diff/2001/state/state.go File state/state.go (right): https://codereview.appspot.com/5502043/diff/2001/state/state.go#newcode29 state/state.go:29: The function still looks good, but please remove the two spaces in this function. They're not contributing to the code readability. https://codereview.appspot.com/5502043/diff/2001/state/state.go#newcode38 state/state.go:38: err := s.topology.execute(func(td *topologyData) error { Please check the previous review out. This was already covered there. https://codereview.appspot.com/5502043/diff/2001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/2001/state/topology.go#newcode18 state/topology.go:18: type commandFunc func(*topologyData) error Please check the previous review regarding topology.go.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
This is looking a lot nicer, thanks Frank. There are a substantial amount of review points, but most of them are clearly about alignment rather than significant problems. I'm sure the volume for further branches will be significantly lower. https://codereview.appspot.com/5502043/diff/10/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/10/state/service.go#newcode19 state/service.go:19: func newService(t *topology, id, name string) *Service { Please drop this function. It's easier, clearer, and shorter to use the actual type. https://codereview.appspot.com/5502043/diff/10/state/service.go#newcode37 state/service.go:37: Drop this space. The logic below is directly related to the line above. I'm being a bit more pedantic on this since I noted you tend to include too much spacing within functions, but once we get to a reasonable agreement I'll stop bothering you about this. https://codereview.appspot.com/5502043/diff/10/state/service.go#newcode44 state/service.go:44: return "", ErrServiceHasNoCharmId This is a serious inconsistency, and we likely won't be implementing custom logic for that kind of thing, which means it's not necessary to be exporting errors like that. Return the error in place instead: return "", fmt.Errorf(`service %q has no "charm" field`, s.id) https://codereview.appspot.com/5502043/diff/10/state/state.go File state/state.go (right): https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode6 state/state.go:6: // state data stored in the passed ZooKeeper connection. // The state package enables reading, observing, and changing // the state stored in zookeeper of a whole environment // managed by juju. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode9 state/state.go:9: // automatically using a gozk watch. Drop this. It's documenting a private type and implementation details. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode17 state/state.go:17: // of the parts of the managed environmens. This was in the previous review. Please watch out for silently ignoring points made. // State enables reading, observing, and changing the state // of a whole environment managed by juju. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode22 state/state.go:22: // Open returns a new instance of the state. Ditto. // Open returns a new State representing the environment // being accessed through the zk connection. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode32 state/state.go:32: func (s *State) Service(n string) (*Service, error) { s/n/name/, arguments are part of the documentation, so we need to be a bit more careful about them, even more when the type gives no hints about what it is. https://codereview.appspot.com/5502043/diff/10/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode5 state/state_test.go:5: package state Please name this package as state_test, and import the state package explicitly below. Tests should be blackbox by default, unless there's a strong need for whiteboxing. This isn't the case here. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode50 state/state_test.go:50: create("/topology", testTopology) Inline the state creation on the test you actually want it. Different tests will need different states, and many of them will need very little state that is nothing like this. Having a fixed fake state generally leads to tests that are both hard to understand and to maintain over time once they increase in number. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode76 state/state_test.go:76: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode78 state/state_test.go:78: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode82 state/state_test.go:82: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode84 state/state_test.go:84: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode90 state/state_test.go:90: time.Sleep(30e9) Wow.. waiting for 30s just sitting and doing nothing is totally unacceptable for a test case, and unnecessary in this case. See how the gozk tests are done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode93 state/state_test.go:93: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode110 state/state_test.go:110: c.Log("Test service ...") Drop log comment. It's adding nothing in addition to the method name. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode115 state/state_test.go:115: var charmId string Drop variable block. Use := below. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode118 state/state_test.go:118: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode120 state/state_test.go:120: c.Assert(state, Not(IsNil)) Please drop the Not(IsNil) checks. If this blows up, the error message will give us nothing useful, so we may as well trust that err == nil means service is not nil, and use it below. If it is nil, this test will necessarily fail. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode123 state/state_test.go:123: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode125 state/state_test.go:125: c.Assert(service, Not(IsNil)) Same as above. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode128 state/state_test.go:128: Drop empty line. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode131 state/state_test.go:131: } The test here looks very good, but there is functionality in the code base right now that is uncovered. Please check out which functions and which public types and methods are implemented, and make sure they're covered here too. https://codereview.appspot.com/5502043/diff/10/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode16 state/topology.go:16: // ZkTopology is the data stored in ZK below "/topology". It's s/ZK/ZooKeeper/ in the documentation. s/below/inside/ https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode18 state/topology.go:18: // ZkXyz types sadly have to be public for this. No, they don't have to be public. The field names have to be exported, not the types: http://paste.ubuntu.com/779866/ Please drop the comment and rename all Zk* types to zk*. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode27 state/topology.go:27: func newZkTopology() *ZkTopology { Drop this function. The make calls are not necessary now that goyaml is fixed, and creating it inline is done once and much nicer: &zkTopology{Version: 1} https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode56 state/topology.go:56: zk *zookeeper.Conn So far, it doesn't need the connection (see comments below). Please remove it from the type for now, and use it in readTopology only. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode63 state/topology.go:63: func newTopology(zk *zookeeper.Conn) (*topology, error) { s/newTopology/readTopology/ https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode76 state/topology.go:76: func (t *topology) getStringMap(path string) (map[string]string, error) { This is unrelated to the topology. Move it to a function taking (zk, path) as arguments. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode92 state/topology.go:92: func (t *topology) reload() error { Please drop this method and move the functionality to readTopology, fixing the logic elsewhere to work without it. The key point why we have a topology in the first place is to enable transactional behavior for some settings that must be in agreement. If you reload the topology every time its methods are touched, it means we lose precisely that benefit. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode118 state/topology.go:118: if err := t.reload(); err != nil { See above. https://codereview.appspot.com/5502043/diff/10/state/unit.go File state/unit.go (right): https://codereview.appspot.com/5502043/diff/10/state/unit.go#newcode1 state/unit.go:1: // launchpad.net/juju/state Please remove this file for now. It looks good, but it's not part of this branch. https://codereview.appspot.com/5502043/diff/10/state/util.go File state/util.go (right): https://codereview.appspot.com/5502043/diff/10/state/util.go#newcode12 state/util.go:12: ErrIncompatibleVersion = errors.New("state: loaded topology has incompatible version") Please drop this file, and inline all of these error messages in their respective call sites, with more details about the error. For example: return fmt.Errorf("incompatible topology version: want %d, got %d", want, got) return fmt.Errorf("service %q was not found", name) etc. We can have actual error types when we actually feel the need for them, but right now it's augmenting the public API unnecessarily, and providing a lesser result with worse error messages.
Sign in to reply to this message.
looks good generally. a couple of hopefully-not-too-bikesheddy remarks on some commentary. https://codereview.appspot.com/5502043/diff/10/state/state.go File state/state.go (right): https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode17 state/state.go:17: // of the parts of the managed environmens. On 2011/12/23 15:08:30, niemeyer wrote: > This was in the previous review. Please watch out for silently ignoring points > made. > > // State enables reading, observing, and changing the state > // of a whole environment managed by juju. How about "State represents the state of a juju environment"? https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode22 state/state.go:22: // Open returns a new instance of the state. On 2011/12/23 15:08:30, niemeyer wrote: > Ditto. > > // Open returns a new State representing the environment > // being accessed through the zk connection. "Open returns a new State representing a juju environment, accessed through the given zk connection"?
Sign in to reply to this message.
>> // State enables reading, observing, and changing the state >> // of a whole environment managed by juju. > > How about "State represents the state of a juju environment"? Still prefer the previously suggested version. >> // Open returns a new State representing the environment >> // being accessed through the zk connection. > > "Open returns a new State representing a juju environment, accessed > through the given zk connection"? Sounds good. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 23 December 2011 18:05, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >>> // State enables reading, observing, and changing the state >>> // of a whole environment managed by juju. >> >> How about "State represents the state of a juju environment"? > > Still prefer the previously suggested version. my thought was that the methods themselves say what can be done with the environment, and that the commentary on the type only needs to say what that type is, or represents. looking at the Go standard library, after a brief look, i can't see any type comments similar to the above. it doesn't read quite right to my eyes, although i agree with what it's trying to say. if we wanted to keep the same information in the comment, we could go to two sentences. how about this? "State represents the state of an environment managed by juju. It provides methods to read, observe and change that state."
Sign in to reply to this message.
>>>> // State enables reading, observing, and changing the state >>>> // of a whole environment managed by juju. > > my thought was that the methods themselves say what can > be done with the environment, and that the commentary on the type The vast majority of type documentation is talking about what its methods allow a developer to do with the type, and the comment above is not getting into any specifics that could be regarded as method documentation, so I can't follow your thinking. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 23 December 2011 20:12, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >>>>> // State enables reading, observing, and changing the state >>>>> // of a whole environment managed by juju. >> >> my thought was that the methods themselves say what can >> be done with the environment, and that the commentary on the type > > The vast majority of type documentation is talking about what its > methods allow a developer to do with the type, and the comment above > is not getting into any specifics that could be regarded as method > documentation, so I can't follow your thinking. Looking at the documentation on the type declaration itself, the initial sentence of the vast majority talks about what the type *is*, not what the type does, and still less what the type enables you do to do. (for reference, here's a list of all the relevant doc lines extracted from $GOROOT/src/pkg: http://paste.ubuntu.com/791700/; it's the first line mentioning the type name for each type with a comment) I'm simply suggesting that the comment might be rewritten to be more in line with this usual practice. "State enables..." doesn't quite feel right to me.
Sign in to reply to this message.
https://codereview.appspot.com/5502043/diff/10/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/10/state/service.go#newcode19 state/service.go:19: func newService(t *topology, id, name string) *Service { On 2011/12/23 15:08:30, niemeyer wrote: > Please drop this function. It's easier, clearer, and shorter to use the actual > type. Done. https://codereview.appspot.com/5502043/diff/10/state/service.go#newcode37 state/service.go:37: On 2011/12/23 15:08:30, niemeyer wrote: > Drop this space. The logic below is directly related to the line above. I'm > being a bit more pedantic on this since I noted you tend to include too much > spacing within functions, but once we get to a reasonable agreement I'll stop > bothering you about this. Done. https://codereview.appspot.com/5502043/diff/10/state/service.go#newcode44 state/service.go:44: return "", ErrServiceHasNoCharmId On 2011/12/23 15:08:30, niemeyer wrote: > This is a serious inconsistency, and we likely won't be implementing custom > logic for that kind of thing, which means it's not necessary to be exporting > errors like that. Return the error in place instead: > > return "", fmt.Errorf(`service %q has no "charm" field`, s.id) Done. https://codereview.appspot.com/5502043/diff/10/state/state.go File state/state.go (right): https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode6 state/state.go:6: // state data stored in the passed ZooKeeper connection. On 2011/12/23 15:08:30, niemeyer wrote: > // The state package enables reading, observing, and changing > // the state stored in zookeeper of a whole environment > // managed by juju. Done. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode9 state/state.go:9: // automatically using a gozk watch. On 2011/12/23 15:08:30, niemeyer wrote: > Drop this. It's documenting a private type and implementation details. Done. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode17 state/state.go:17: // of the parts of the managed environmens. On 2011/12/23 15:08:30, niemeyer wrote: > This was in the previous review. Please watch out for silently ignoring points > made. > > // State enables reading, observing, and changing the state > // of a whole environment managed by juju. Done. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode22 state/state.go:22: // Open returns a new instance of the state. On 2011/12/23 15:08:30, niemeyer wrote: > Ditto. > > // Open returns a new State representing the environment > // being accessed through the zk connection. Done. https://codereview.appspot.com/5502043/diff/10/state/state.go#newcode32 state/state.go:32: func (s *State) Service(n string) (*Service, error) { On 2011/12/23 15:08:30, niemeyer wrote: > s/n/name/, arguments are part of the documentation, so we need to be a bit more > careful about them, even more when the type gives no hints about what it is. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode5 state/state_test.go:5: package state On 2011/12/23 15:08:30, niemeyer wrote: > Please name this package as state_test, and import the state package explicitly > below. Tests should be blackbox by default, unless there's a strong need for > whiteboxing. This isn't the case here. Naming the package in the testing file like the tested package is standard in Go (see .../src/pkg). The tools handle them so that they aren't compiled and linked outside the testrun. godoc also ignores test files, so it doesn't show any exported names. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode50 state/state_test.go:50: create("/topology", testTopology) On 2011/12/23 15:08:30, niemeyer wrote: > Inline the state creation on the test you actually want it. Different tests will > need different states, and many of them will need very little state that is > nothing like this. Having a fixed fake state generally leads to tests that are > both hard to understand and to maintain over time once they increase in number. That's obvious. It's just a temporary starting point to get warm with ZK. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode76 state/state_test.go:76: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode78 state/state_test.go:78: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode82 state/state_test.go:82: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode84 state/state_test.go:84: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode90 state/state_test.go:90: time.Sleep(30e9) On 2011/12/23 15:08:30, niemeyer wrote: > Wow.. waiting for 30s just sitting and doing nothing is totally unacceptable for > a test case, and unnecessary in this case. See how the gozk tests are done. Done, but as a first step with a simpler connection handling. Maybe you could give me a little intro in ZK and gozk in Budapest, especially why you're buffering the events in your tests. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode93 state/state_test.go:93: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode110 state/state_test.go:110: c.Log("Test service ...") On 2011/12/23 15:08:30, niemeyer wrote: > Drop log comment. It's adding nothing in addition to the method name. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode115 state/state_test.go:115: var charmId string On 2011/12/23 15:08:30, niemeyer wrote: > Drop variable block. Use := below. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode118 state/state_test.go:118: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode120 state/state_test.go:120: c.Assert(state, Not(IsNil)) On 2011/12/23 15:08:30, niemeyer wrote: > Please drop the Not(IsNil) checks. If this blows up, the error message will give > us nothing useful, so we may as well trust that err == nil means service is not > nil, and use it below. If it is nil, this test will necessarily fail. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode123 state/state_test.go:123: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode125 state/state_test.go:125: c.Assert(service, Not(IsNil)) On 2011/12/23 15:08:30, niemeyer wrote: > Same as above. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode128 state/state_test.go:128: On 2011/12/23 15:08:30, niemeyer wrote: > Drop empty line. Done. https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode131 state/state_test.go:131: } On 2011/12/23 15:08:30, niemeyer wrote: > The test here looks very good, but there is functionality in the code base right > now that is uncovered. Please check out which functions and which public types > and methods are implemented, and make sure they're covered here too. Of course. It just has been the current state to give a chance for a review before the break. https://codereview.appspot.com/5502043/diff/10/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode16 state/topology.go:16: // ZkTopology is the data stored in ZK below "/topology". It's On 2011/12/23 15:08:30, niemeyer wrote: > s/ZK/ZooKeeper/ in the documentation. > s/below/inside/ Done. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode18 state/topology.go:18: // ZkXyz types sadly have to be public for this. On 2011/12/23 15:08:30, niemeyer wrote: > No, they don't have to be public. The field names have to be exported, not the > types: http://paste.ubuntu.com/779866/ > > Please drop the comment and rename all Zk* types to zk*. Done. Great hint, thx. Until today I only had to marshal public types, so I didn't know it. Fine. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode27 state/topology.go:27: func newZkTopology() *ZkTopology { On 2011/12/23 15:08:30, niemeyer wrote: > Drop this function. The make calls are not necessary now that goyaml is fixed, > and creating it inline is done once and much nicer: > > &zkTopology{Version: 1} Done. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode56 state/topology.go:56: zk *zookeeper.Conn On 2011/12/23 15:08:30, niemeyer wrote: > So far, it doesn't need the connection (see comments below). Please remove it > from the type for now, and use it in readTopology only. Done. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode63 state/topology.go:63: func newTopology(zk *zookeeper.Conn) (*topology, error) { On 2011/12/23 15:08:30, niemeyer wrote: > s/newTopology/readTopology/ Done. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode76 state/topology.go:76: func (t *topology) getStringMap(path string) (map[string]string, error) { On 2011/12/23 15:08:30, niemeyer wrote: > This is unrelated to the topology. Move it to a function taking (zk, path) as > arguments. Done. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode92 state/topology.go:92: func (t *topology) reload() error { On 2011/12/23 15:08:30, niemeyer wrote: > Please drop this method and move the functionality to readTopology, fixing the > logic elsewhere to work without it. The key point why we have a topology in the > first place is to enable transactional behavior for some settings that must be > in agreement. If you reload the topology every time its methods are touched, it > means we lose precisely that benefit. Done. https://codereview.appspot.com/5502043/diff/10/state/topology.go#newcode118 state/topology.go:118: if err := t.reload(); err != nil { On 2011/12/23 15:08:30, niemeyer wrote: > See above. Done. https://codereview.appspot.com/5502043/diff/10/state/util.go File state/util.go (right): https://codereview.appspot.com/5502043/diff/10/state/util.go#newcode12 state/util.go:12: ErrIncompatibleVersion = errors.New("state: loaded topology has incompatible version") On 2011/12/23 15:08:30, niemeyer wrote: > Please drop this file, and inline all of these error messages in their > respective call sites, with more details about the error. For example: > > return fmt.Errorf("incompatible topology version: want %d, got %d", want, > got) > > return fmt.Errorf("service %q was not found", name) > > etc. > > We can have actual error types when we actually feel the need for them, but > right now it's augmenting the public API unnecessarily, and providing a lesser > result with worse error messages. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks for the changes, Frank. Looking forward to meet next week. Meanwhile, just a minor to get you going: https://codereview.appspot.com/5502043/diff/10/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode5 state/state_test.go:5: package state > Naming the package in the testing file like the tested package is standard in Go > (see .../src/pkg). The tools handle them so that they aren't compiled and linked > outside the testrun. godoc also ignores test files, so it doesn't show any > exported names. I understand it's done in many Go packages, and my comment takes that into consideration as well. Please re-read the comment and proceed in light of that.
Sign in to reply to this message.
Thanks for the changes, Frank. Looking forward to meet next week. Meanwhile, just a minor to get you going: https://codereview.appspot.com/5502043/diff/10/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode5 state/state_test.go:5: package state > Naming the package in the testing file like the tested package is standard in Go > (see .../src/pkg). The tools handle them so that they aren't compiled and linked > outside the testrun. godoc also ignores test files, so it doesn't show any > exported names. I understand it's done in many Go packages, and my comment takes that into consideration as well. Please re-read the comment and proceed in light of that.
Sign in to reply to this message.
Hi Gustavo, I'll change it and I'm looking forward to our meeting too. CU mue https://codereview.appspot.com/5502043/diff/10/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/10/state/state_test.go#newcode5 state/state_test.go:5: package state On 2012/01/04 17:47:00, niemeyer wrote: > I understand it's done in many Go packages, and my comment > takes that into consideration as well. Please re-read the > comment and proceed in light of that. OK, just to get your intention right: By using a different namespace (package) you want to enforce the blackboxing instead of doing it by convention? That's reasonable and I'll change it.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5502043/diff/12001/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode21 state/service.go:21: // Id returns the service id. Please rename the s.id field to s.node and the Id method to zkNodeName and move the latter to the bottom of the file, as we discussed live. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode22 state/service.go:22: func (s Service) Id() string { s/Service/*Service/, in all of the method receivers. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode26 state/service.go:26: // Name returns the service name. This is what we'll be using in pretty much all places outside of the state package itself. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode53 state/service.go:53: // Define the add function for topology and call it. Please drop this comment. It's transcribing exactly what the code is doing, not how or why, so it's just a distraction rather than being a useful insight. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode54 state/service.go:54: unitId := strings.Split(path, "/")[2] s/unitId/nodeName/ please. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode58 state/service.go:58: return newError("service state has changed") This is a fairly common error we use across the board in the state code. Please define an internal stateChanged error as: stateChanged = errors.New("environment state has changed") and use it here. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode69 state/service.go:69: return &Unit{s.zk, unitId, s.id, s.name, sequenceNo}, nil Check for nodeName here. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode74 state/service.go:74: // First unassign from machine if currently assigned. Point out why it's fine to ignore the error in this call. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode76 state/service.go:76: // Define the remove function fir topology and call it. Drop the comment, as per the reasoning above. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode79 state/service.go:79: return newError("service state has changed") stateChanged https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode92 state/service.go:92: // Unit returns a unit by name. // Unit returns the service's Unit with name. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode93 state/service.go:93: func (s Service) Unit(unitName string) (*Unit, error) { s/unitName/name/ https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode100 state/service.go:100: return nil, newError("service name '%v' of unit does not match with service name '%v'", s/".*"/"can't find unit %q on service %q", unitName, s.name) https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode103 state/service.go:103: // Check if the topology has been changed. Drop this comment, as explained. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode109 state/service.go:109: return nil, newError("service state has changed") stateChanged https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode112 state/service.go:112: unitId, err := topology.unitIdBySequence(s.id, sequenceNo) nodeName across the board here, I think? https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode121 state/service.go:121: // Check if the topology has changed. This comment is misplaced. This is just retrieving the topology because we need it. It could be moved to right before the !topology.hasService test, but it feels like it can go away entirely since the code is clear. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode127 state/service.go:127: return nil, newError("service state has changed") stateChanged https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode129 state/service.go:129: // Retrieve unit ids. Please drop comment. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode130 state/service.go:130: unitIds, err := topology.unitIds(s.id) unitIds or nodeNames? https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode150 state/service.go:150: // UnitNames returns the names of the services units. // UnitNames returns the names of all units of service. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode151 state/service.go:151: func (s Service) UnitNames() ([]string, error) { You got all the names nicely, thanks. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode158 state/service.go:158: return nil, newError("service state has changed") stateChanged https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode160 state/service.go:160: // Retrieve unit ids. Drop comment. It's mimicking the code. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode161 state/service.go:161: unitIds, err := topology.unitIds(s.id) unitIds or nodeNames? https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode184 state/service.go:184: return fmt.Sprintf("%s/config", s.zkPath()) For consistency with the method below, please define this as: return fmt.Sprintf("/services/%s/config", s.node) https://codereview.appspot.com/5502043/diff/12001/state/state.go File state/state.go (right): https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode15 state/state.go:15: // of a whole environment managed by juju. For settling on the whole discussion: // State represents the state of an environment // managed by juju. I'm sure rog will be happy. :) https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode32 state/state.go:32: func (s *State) Service(serviceName string) (*Service, error) { s/serviceName/name/ https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode33 state/state.go:33: id, err := s.topology.serviceIdByName(serviceName) s/id/node/ and s/serviceId/serviceNode/, I think? https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode41 state/state.go:41: func (s *State) Unit(unitName string) (*Unit, error) { s/unitName/name/ https://codereview.appspot.com/5502043/diff/12001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode15 state/state_test.go:15: // zkCreate is a simple helper to create test scenarios in ZooKeeper. We need a better comment here. It's not clear what this function does, or what its arguments are (please rename p and v, and make c the first argument as this is a common convention for gocheck helpers). https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode27 state/state_test.go:27: // StateSuite for State and the related types. Drop comment. What's being pointed out is implicit from the type name. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode41 state/state_test.go:41: // SetUpSuite starts and inits ZooKeeper. Drop comment. It's not helping much, and it's already out of date (init isn't being done here, and creating the server isn't mentioned, and it probably shouldn't be). https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode49 state/state_test.go:49: // Create server. Drop comment. ("//Create server... CreateServer"!) https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode55 state/state_test.go:55: // Start server. Drop comment. ("// Start server...zkServer.Start"!) https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode62 state/state_test.go:62: // TearDownSuite stops ZooKeeper. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode70 state/state_test.go:70: // event channel. Nice comment. Just a minor: s/returns/and returns/ https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode84 state/state_test.go:84: // done closes a ZooKeeper connection. // done removes potentially created ZooKeeper nodes // recursively and then closes the ZooKeeper connection. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode93 state/state_test.go:93: // TestReadService tests reading operations on services. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode107 state/state_test.go:107: // Open state. Drop comment. ("// Open state... state.Open"!) https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode108 state/state_test.go:108: jujuState, err := state.Open(zk) Can we standardize on "st" for the state to avoid the name clash? I wouldn't like to call it jujuState everywhere. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode111 state/state_test.go:111: // Retrieve legal service. This is what this test is really about, so let's be more explicit: // Check that retrieving a service works correctly. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode117 state/state_test.go:117: // Retrieve charm id of legal service. Drop comment, join line to above block. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode122 state/state_test.go:122: // Retrieve illegal service. // Check that retrieving a non-existent service fails nicely. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode124 state/state_test.go:124: c.Assert(err, Not(IsNil)) err, ErrorMatches, "..." https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode127 state/state_test.go:127: // TestReadUnit tests reading on units. Drop comment. We generally don't comment on tests unless they're adding interesting information, since that won't end up in the generated documentation, and "tests reading on units" has no further insight on top of TestReadUnit. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode151 state/state_test.go:151: // Open state. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode155 state/state_test.go:155: // Retrieve legal service. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode159 state/state_test.go:159: // Retrieve legal unit. // Check that retrieving a unit works correctly. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode165 state/state_test.go:165: // Retrieve illegal unit names and illegal units. // Check that retrieving a non-existent or an invalidly // named unit fail nicely. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode168 state/state_test.go:168: c.Assert(err.Error(), Equals, "state: 'wordpress' is no valid unit name") Drop the Not(IsNil) line and use this: c.Assert(err, ErrorMatches, "...") https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode171 state/state_test.go:171: c.Assert(err.Error(), Equals, "state: 'wordpress/0/0' is no valid unit name") ErrorMatches https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode174 state/state_test.go:174: c.Assert(err, Not(IsNil)) ErrorMatches https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode176 state/state_test.go:176: // Retrieve legal unit directly from state. // Check that retrieving a unit works. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode182 state/state_test.go:182: // Retrieve all unit names. // Check that retrieving unit names works. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode187 state/state_test.go:187: // Retrieve all units. // Check that retrieving all units works. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode193 state/state_test.go:193: // TestWriteUnit tests writing on units. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode220 state/state_test.go:220: // Open state and get a service. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode226 state/state_test.go:226: // Add a unit. // Check that adding a unit works. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode232 state/state_test.go:232: // Remove a legal unit. // Check that removing a unit works. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode241 state/state_test.go:241: // Remove an illegal unit. // Check that removing a non-existent unit fails nicely. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode259 state/state_test.go:259: // Now delete the path itself. Just as a data point for comparison, this function has a good comment at the top, and good comments inside it. https://codereview.appspot.com/5502043/diff/12001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode13 state/topology.go:13: const ( Please rename Version to topologyVersion (making it private for now), and copy the comment from the Python code: // The protocol version, which is stored in the /topology node under // the "version" key. The protocol version should *only* be updated // when we know that a version is in fact actually incompatible. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode19 state/topology.go:19: // ZkXyz types sadly have to be public for this. Replace the whole comment by: // zkTopology is used to marshal and unmarshal the content // of the /topology node in ZooKeeper. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode27 state/topology.go:27: // internally for marshalling/unmarshalling. // zkService represents the service data within the /topology // node in ZooKeeper. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode34 state/topology.go:34: // for marshalling/unmarshalling. // zkUnit represents the unit data within the /topology // node in ZooKeeper. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode41 state/topology.go:41: // inside of ZooKeeper. // topology is an internal helper that handles the content // of the /topology node in ZooKeeper. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode50 state/topology.go:50: // Fetch raw topology. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode51 state/topology.go:51: topologyYaml, _, err := zk.Get("/topology") We have enough context, so: s/topologyYaml/yaml/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode55 state/topology.go:55: // Parse and check it. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode62 state/topology.go:62: // parse parses the topology delivered as YAML. This should be a function returning a *topology rather than a method on *topology. Please use this comment: // parseTopology returns the topology represented by yaml. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode63 state/topology.go:63: func (t topology) parse(topologyYaml string) error { s/topologyYaml/yaml/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode68 state/topology.go:68: return newError("loaded topology has incompatible version '%v'", t.topology.Version) return fmt.Errorf("incompatible topology versions: got %d, want %d", t.topology.Version, Version) https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode73 state/topology.go:73: // dump returns the topology as YAML. This makes sense as a method, though. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode74 state/topology.go:74: func (t topology) dump() (string, error) { Please replace all receivers: s/t topology/t *topology/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode93 state/topology.go:93: func (t topology) serviceIdByName(name string) (string, error) { s/serviceIdByName/serviceNodeName/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode99 state/topology.go:99: return "", newError("service with name '%s' cannot be found", name) fmt.Errorf https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode103 state/topology.go:103: func (t topology) hasUnit(serviceId, unitId string) bool { s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode112 state/topology.go:112: func (t *topology) addUnit(serviceId, unitId string) (int, error) { s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode119 state/topology.go:119: return -1, newError("unit id '%s' already in use in servie '%s'", unitId, id) fmt.Errorf s/'%v'/%q/g https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode131 state/topology.go:131: func (t *topology) removeUnit(serviceId, unitId string) error { s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode139 state/topology.go:139: // unitIds returns the ids of all units of a service. // unitNodes returns the unit node names for all units of // the service with serviceNode node name. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode140 state/topology.go:140: func (t topology) unitIds(serviceId string) ([]string, error) { s/unitIds/unitNodes/ s/serviceId/serviceNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode154 state/topology.go:154: func (t topology) unitName(serviceId, unitId string) (string, error) { s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode165 state/topology.go:165: func (t topology) unitIdBySequence(serviceId string, sequenceNo int) (string, error) { s/unitIdBySequence/unitNodeFromSequence/ s/serviceId/serviceNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode170 state/topology.go:170: for id, unit := range svc.Units { s/id/node/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode175 state/topology.go:175: return "", newError("unit with sequence number '%v' cannot be found", sequenceNo) fmt.Errorf s/'%v'/%d/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode180 state/topology.go:180: func (t *topology) unitMachineId(serviceId, unitId string) (string, error) { Please check if this is an external machine id or an internal machine id. I suspect it's an internal one, in which case: s/unitMachineId/unitMachineNode/ s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode189 state/topology.go:189: func (t *topology) unassignUnitFromMachine(serviceId, unitId string) error { s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode195 state/topology.go:195: return newError("unit '%s' in service '%s' is not assigned to a machine", unitId, serviceId) fmt.Errorf s/'%s'/%q/g https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode202 state/topology.go:202: func (t topology) assertService(serviceId string) error { s/serviceId/serviceNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode204 state/topology.go:204: return newError("service with id '%v' cannot be found", serviceId) fmt.Errorf s/'%v'/%q/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode210 state/topology.go:210: func (t topology) assertUnit(serviceId, unitId string) error { s/serviceId/serviceNode/ s/unitId/unitNode/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode216 state/topology.go:216: return newError("unit with id '%v' cannot be found", serviceId) fmt.Errorf s/'%v'/%q/ https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode222 state/topology.go:222: // accepts a topology instance as an argument. This function can read // retryTopologyChange tries to change the topology with f. // This function can ... (no need to document redundant typing information) https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode225 state/topology.go:225: // persisted into the /topology node. Note that this function must // ... Note that f must ... https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode230 state/topology.go:230: changeContent := func(topologyYaml string, stat *zookeeper.Stat) (string, error) { We know that within this function we're working to retryTopologyChange, so we can trust on that context to use shorter variable names where the meaning is clear, making the function more readable: s/changeContent/change/ s/topologyYaml/yaml/ https://codereview.appspot.com/5502043/diff/12001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode15 state/unit.go:15: // and its subordinate parts. // Unit represents the state of a service unit. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode18 state/unit.go:18: id string s/id/node/ https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode19 state/unit.go:19: serviceId string s/serviceId/serviceNode/ https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode30 state/unit.go:30: func (u Unit) Id() string { s/Id/zkNodeName/, and move to bottom next to the similar functions. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode35 state/unit.go:35: // name and the sequence number. // Name returns the unit name. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode41 state/unit.go:41: // any machine it's assigned to. s/any machine/the machine/ https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode45 state/unit.go:45: return newError("service state has changed") stateChanged https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode49 state/unit.go:49: // have to deal with conflicts. This sounds sensible, but I don't think it's what the logic below is doing. Please fix it below and watch out for those cases. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode75 state/unit.go:75: // sequence number. You're adding unnecessary typing information to the comment. Please change the function declaration to this: parseUnitName(name string) (serviceName string, seqNo int, err error) And use this comment: // parseUnitName parses a unit name like "wordpress/0" into // its service name and sequence number parts. https://codereview.appspot.com/5502043/diff/12001/state/util.go File state/util.go (right): https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode14 state/util.go:14: // newError allows a quick error creation with arguments. Drop this function and use fmt.Errorf. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode20 state/util.go:20: // based on a path. s/zkStringMap/zkMap/, for the reason stated below, and use the following comment: // zkMap reads the yaml data of path from zk and // parses it into a map. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode21 state/util.go:21: func zkStringMap(zk *zookeeper.Conn, path string) (map[string]string, error) { The map type here is a problem. yaml has typing information, so if you unmarshal an int, it should be marshalled back as an int rather than as a string. Make this map: map[string]interface{}, and do the casting as appropriate (checking for errors!). https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode22 state/util.go:22: // Fetch raw data. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode27 state/util.go:27: // Unmarshal it. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode35 state/util.go:35: // zkStringMapField returns a field our of a string map returned by stringMap(). Turn this into zkMapField, per the logic above, and fix the comment: // zkMapField reads path from zk as a yaml map, and returns // the value for field. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode36 state/util.go:36: func zkStringMapField(zk *zookeeper.Conn, path, field string) (string, error) { Use named results: (value interface{}, err error) https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode37 state/util.go:37: // Get the map. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode42 state/util.go:42: // Look if field exists. Drop comment. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode45 state/util.go:45: return "", newError("cannot find field '%s' in path '%s'", field, path) fmt.Errorf s/'%s'/%q/g https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode52 state/util.go:52: // First recursively delete the cildren. Good comments! Except for a minor typo: s/cildren/children/
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/5502043/diff/12001/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode21 state/service.go:21: // Id returns the service id. On 2012/01/10 13:02:17, niemeyer wrote: > Please rename the s.id field to s.node and the Id method to zkNodeName and move > the latter to the bottom of the file, as we discussed live. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode22 state/service.go:22: func (s Service) Id() string { On 2012/01/10 13:02:17, niemeyer wrote: > s/Service/*Service/, in all of the method receivers. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode26 state/service.go:26: // Name returns the service name. On 2012/01/10 13:02:17, niemeyer wrote: > This is what we'll be using in pretty much all places outside of the state > package itself. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode53 state/service.go:53: // Define the add function for topology and call it. On 2012/01/10 13:02:17, niemeyer wrote: > Please drop this comment. It's transcribing exactly what the code is doing, not > how or why, so it's just a distraction rather than being a useful insight. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode54 state/service.go:54: unitId := strings.Split(path, "/")[2] On 2012/01/10 13:02:17, niemeyer wrote: > s/unitId/nodeName/ please. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode58 state/service.go:58: return newError("service state has changed") On 2012/01/10 13:02:17, niemeyer wrote: > This is a fairly common error we use across the board in the state code. Please > define an internal stateChanged error as: > > stateChanged = errors.New("environment state has changed") > > and use it here. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode69 state/service.go:69: return &Unit{s.zk, unitId, s.id, s.name, sequenceNo}, nil On 2012/01/10 13:02:17, niemeyer wrote: > Check for nodeName here. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode74 state/service.go:74: // First unassign from machine if currently assigned. On 2012/01/10 13:02:17, niemeyer wrote: > Point out why it's fine to ignore the error in this call. Done. Just layouted the method based on the Py code and missed the raised error here. Funnily the method itself returns a possible error. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode76 state/service.go:76: // Define the remove function fir topology and call it. On 2012/01/10 13:02:17, niemeyer wrote: > Drop the comment, as per the reasoning above. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode79 state/service.go:79: return newError("service state has changed") On 2012/01/10 13:02:17, niemeyer wrote: > stateChanged Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode92 state/service.go:92: // Unit returns a unit by name. On 2012/01/10 13:02:17, niemeyer wrote: > // Unit returns the service's Unit with name. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode93 state/service.go:93: func (s Service) Unit(unitName string) (*Unit, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/unitName/name/ Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode100 state/service.go:100: return nil, newError("service name '%v' of unit does not match with service name '%v'", On 2012/01/10 13:02:17, niemeyer wrote: > s/".*"/"can't find unit %q on service %q", unitName, s.name) Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode103 state/service.go:103: // Check if the topology has been changed. On 2012/01/10 13:02:17, niemeyer wrote: > Drop this comment, as explained. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode109 state/service.go:109: return nil, newError("service state has changed") On 2012/01/10 13:02:17, niemeyer wrote: > stateChanged Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode112 state/service.go:112: unitId, err := topology.unitIdBySequence(s.id, sequenceNo) On 2012/01/10 13:02:17, niemeyer wrote: > nodeName across the board here, I think? Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode121 state/service.go:121: // Check if the topology has changed. On 2012/01/10 13:02:17, niemeyer wrote: > This comment is misplaced. This is just retrieving the topology because we need > it. It could be moved to right before the !topology.hasService test, but it > feels like it can go away entirely since the code is clear. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode127 state/service.go:127: return nil, newError("service state has changed") On 2012/01/10 13:02:17, niemeyer wrote: > stateChanged Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode129 state/service.go:129: // Retrieve unit ids. On 2012/01/10 13:02:17, niemeyer wrote: > Please drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode130 state/service.go:130: unitIds, err := topology.unitIds(s.id) On 2012/01/10 13:02:17, niemeyer wrote: > unitIds or nodeNames? Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode150 state/service.go:150: // UnitNames returns the names of the services units. On 2012/01/10 13:02:17, niemeyer wrote: > // UnitNames returns the names of all units of service. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode151 state/service.go:151: func (s Service) UnitNames() ([]string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > You got all the names nicely, thanks. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode158 state/service.go:158: return nil, newError("service state has changed") On 2012/01/10 13:02:17, niemeyer wrote: > stateChanged Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode160 state/service.go:160: // Retrieve unit ids. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. It's mimicking the code. Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode161 state/service.go:161: unitIds, err := topology.unitIds(s.id) On 2012/01/10 13:02:17, niemeyer wrote: > unitIds or nodeNames? Done. https://codereview.appspot.com/5502043/diff/12001/state/service.go#newcode184 state/service.go:184: return fmt.Sprintf("%s/config", s.zkPath()) On 2012/01/10 13:02:17, niemeyer wrote: > For consistency with the method below, please define this as: > > return fmt.Sprintf("/services/%s/config", s.node) Done. https://codereview.appspot.com/5502043/diff/12001/state/state.go File state/state.go (right): https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode15 state/state.go:15: // of a whole environment managed by juju. On 2012/01/10 13:02:17, niemeyer wrote: > For settling on the whole discussion: > > // State represents the state of an environment > // managed by juju. > > I'm sure rog will be happy. :) Done. https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode32 state/state.go:32: func (s *State) Service(serviceName string) (*Service, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceName/name/ Done. https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode33 state/state.go:33: id, err := s.topology.serviceIdByName(serviceName) On 2012/01/10 13:02:17, niemeyer wrote: > s/id/node/ and s/serviceId/serviceNode/, I think? Done. https://codereview.appspot.com/5502043/diff/12001/state/state.go#newcode41 state/state.go:41: func (s *State) Unit(unitName string) (*Unit, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/unitName/name/ Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode15 state/state_test.go:15: // zkCreate is a simple helper to create test scenarios in ZooKeeper. On 2012/01/10 13:02:17, niemeyer wrote: > We need a better comment here. It's not clear what this function does, or what > its arguments are (please rename p and v, and make c the first argument as this > is a common convention for gocheck helpers). Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode27 state/state_test.go:27: // StateSuite for State and the related types. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. What's being pointed out is implicit from the type name. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode41 state/state_test.go:41: // SetUpSuite starts and inits ZooKeeper. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. It's not helping much, and it's already out of date (init isn't > being done here, and creating the server isn't mentioned, and it probably > shouldn't be). Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode49 state/state_test.go:49: // Create server. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. ("//Create server... CreateServer"!) Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode55 state/state_test.go:55: // Start server. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. ("// Start server...zkServer.Start"!) Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode62 state/state_test.go:62: // TearDownSuite stops ZooKeeper. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode70 state/state_test.go:70: // event channel. On 2012/01/10 13:02:17, niemeyer wrote: > Nice comment. > > Just a minor: > > s/returns/and returns/ Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode84 state/state_test.go:84: // done closes a ZooKeeper connection. On 2012/01/10 13:02:17, niemeyer wrote: > // done removes potentially created ZooKeeper nodes > // recursively and then closes the ZooKeeper connection. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode93 state/state_test.go:93: // TestReadService tests reading operations on services. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode107 state/state_test.go:107: // Open state. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. ("// Open state... state.Open"!) Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode108 state/state_test.go:108: jujuState, err := state.Open(zk) On 2012/01/10 13:02:17, niemeyer wrote: > Can we standardize on "st" for the state to avoid the name clash? I wouldn't > like to call it jujuState everywhere. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode111 state/state_test.go:111: // Retrieve legal service. On 2012/01/10 13:02:17, niemeyer wrote: > This is what this test is really about, so let's be more explicit: > > // Check that retrieving a service works correctly. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode117 state/state_test.go:117: // Retrieve charm id of legal service. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment, join line to above block. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode122 state/state_test.go:122: // Retrieve illegal service. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that retrieving a non-existent service fails nicely. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode124 state/state_test.go:124: c.Assert(err, Not(IsNil)) On 2012/01/10 13:02:17, niemeyer wrote: > err, ErrorMatches, "..." Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode127 state/state_test.go:127: // TestReadUnit tests reading on units. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. We generally don't comment on tests unless they're adding > interesting information, since that won't end up in the generated documentation, > and "tests reading on units" has no further insight on top of TestReadUnit. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode151 state/state_test.go:151: // Open state. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode155 state/state_test.go:155: // Retrieve legal service. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode159 state/state_test.go:159: // Retrieve legal unit. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that retrieving a unit works correctly. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode165 state/state_test.go:165: // Retrieve illegal unit names and illegal units. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that retrieving a non-existent or an invalidly > // named unit fail nicely. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode168 state/state_test.go:168: c.Assert(err.Error(), Equals, "state: 'wordpress' is no valid unit name") On 2012/01/10 13:02:17, niemeyer wrote: > Drop the Not(IsNil) line and use this: > > c.Assert(err, ErrorMatches, "...") Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode171 state/state_test.go:171: c.Assert(err.Error(), Equals, "state: 'wordpress/0/0' is no valid unit name") On 2012/01/10 13:02:17, niemeyer wrote: > ErrorMatches Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode174 state/state_test.go:174: c.Assert(err, Not(IsNil)) On 2012/01/10 13:02:17, niemeyer wrote: > ErrorMatches Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode176 state/state_test.go:176: // Retrieve legal unit directly from state. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that retrieving a unit works. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode182 state/state_test.go:182: // Retrieve all unit names. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that retrieving unit names works. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode187 state/state_test.go:187: // Retrieve all units. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that retrieving all units works. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode193 state/state_test.go:193: // TestWriteUnit tests writing on units. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode220 state/state_test.go:220: // Open state and get a service. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode226 state/state_test.go:226: // Add a unit. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that adding a unit works. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode232 state/state_test.go:232: // Remove a legal unit. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that removing a unit works. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode241 state/state_test.go:241: // Remove an illegal unit. On 2012/01/10 13:02:17, niemeyer wrote: > // Check that removing a non-existent unit fails nicely. Done. https://codereview.appspot.com/5502043/diff/12001/state/state_test.go#newcode259 state/state_test.go:259: // Now delete the path itself. On 2012/01/10 13:02:17, niemeyer wrote: > Just as a data point for comparison, this function has a good comment at the > top, and good comments inside it. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode13 state/topology.go:13: const ( On 2012/01/10 13:02:17, niemeyer wrote: > Please rename Version to topologyVersion (making it private for now), and copy > the comment from the Python code: > > // The protocol version, which is stored in the /topology node under > // the "version" key. The protocol version should *only* be updated > // when we know that a version is in fact actually incompatible. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode19 state/topology.go:19: // ZkXyz types sadly have to be public for this. On 2012/01/10 13:02:17, niemeyer wrote: > Replace the whole comment by: > > // zkTopology is used to marshal and unmarshal the content > // of the /topology node in ZooKeeper. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode27 state/topology.go:27: // internally for marshalling/unmarshalling. On 2012/01/10 13:02:17, niemeyer wrote: > // zkService represents the service data within the /topology > // node in ZooKeeper. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode34 state/topology.go:34: // for marshalling/unmarshalling. On 2012/01/10 13:02:17, niemeyer wrote: > // zkUnit represents the unit data within the /topology > // node in ZooKeeper. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode41 state/topology.go:41: // inside of ZooKeeper. On 2012/01/10 13:02:17, niemeyer wrote: > // topology is an internal helper that handles the content > // of the /topology node in ZooKeeper. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode50 state/topology.go:50: // Fetch raw topology. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode51 state/topology.go:51: topologyYaml, _, err := zk.Get("/topology") On 2012/01/10 13:02:17, niemeyer wrote: > We have enough context, so: > > s/topologyYaml/yaml/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode55 state/topology.go:55: // Parse and check it. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode62 state/topology.go:62: // parse parses the topology delivered as YAML. On 2012/01/10 13:02:17, niemeyer wrote: > This should be a function returning a *topology rather than a method on > *topology. Please use this comment: > > // parseTopology returns the topology represented by yaml. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode63 state/topology.go:63: func (t topology) parse(topologyYaml string) error { On 2012/01/10 13:02:17, niemeyer wrote: > s/topologyYaml/yaml/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode68 state/topology.go:68: return newError("loaded topology has incompatible version '%v'", t.topology.Version) On 2012/01/10 13:02:17, niemeyer wrote: > return fmt.Errorf("incompatible topology versions: got %d, want %d", > t.topology.Version, Version) Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode73 state/topology.go:73: // dump returns the topology as YAML. On 2012/01/10 13:02:17, niemeyer wrote: > This makes sense as a method, though. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode74 state/topology.go:74: func (t topology) dump() (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > Please replace all receivers: s/t topology/t *topology/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode93 state/topology.go:93: func (t topology) serviceIdByName(name string) (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceIdByName/serviceNodeName/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode99 state/topology.go:99: return "", newError("service with name '%s' cannot be found", name) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode103 state/topology.go:103: func (t topology) hasUnit(serviceId, unitId string) bool { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode112 state/topology.go:112: func (t *topology) addUnit(serviceId, unitId string) (int, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode119 state/topology.go:119: return -1, newError("unit id '%s' already in use in servie '%s'", unitId, id) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf > s/'%v'/%q/g Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode131 state/topology.go:131: func (t *topology) removeUnit(serviceId, unitId string) error { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode139 state/topology.go:139: // unitIds returns the ids of all units of a service. On 2012/01/10 13:02:17, niemeyer wrote: > // unitNodes returns the unit node names for all units of > // the service with serviceNode node name. Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode140 state/topology.go:140: func (t topology) unitIds(serviceId string) ([]string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/unitIds/unitNodes/ > s/serviceId/serviceNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode154 state/topology.go:154: func (t topology) unitName(serviceId, unitId string) (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode165 state/topology.go:165: func (t topology) unitIdBySequence(serviceId string, sequenceNo int) (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > s/unitIdBySequence/unitNodeFromSequence/ > s/serviceId/serviceNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode170 state/topology.go:170: for id, unit := range svc.Units { On 2012/01/10 13:02:17, niemeyer wrote: > s/id/node/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode175 state/topology.go:175: return "", newError("unit with sequence number '%v' cannot be found", sequenceNo) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf > s/'%v'/%d/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode180 state/topology.go:180: func (t *topology) unitMachineId(serviceId, unitId string) (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > Please check if this is an external machine id or an internal machine id. I > suspect it's an internal one, in which case: > > s/unitMachineId/unitMachineNode/ > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode189 state/topology.go:189: func (t *topology) unassignUnitFromMachine(serviceId, unitId string) error { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode195 state/topology.go:195: return newError("unit '%s' in service '%s' is not assigned to a machine", unitId, serviceId) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf > s/'%s'/%q/g Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode202 state/topology.go:202: func (t topology) assertService(serviceId string) error { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode204 state/topology.go:204: return newError("service with id '%v' cannot be found", serviceId) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf > s/'%v'/%q/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode210 state/topology.go:210: func (t topology) assertUnit(serviceId, unitId string) error { On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ > s/unitId/unitNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode216 state/topology.go:216: return newError("unit with id '%v' cannot be found", serviceId) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf > s/'%v'/%q/ Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode222 state/topology.go:222: // accepts a topology instance as an argument. This function can read On 2012/01/10 13:02:17, niemeyer wrote: > // retryTopologyChange tries to change the topology with f. > // This function can ... > > (no need to document redundant typing information) Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode225 state/topology.go:225: // persisted into the /topology node. Note that this function must On 2012/01/10 13:02:17, niemeyer wrote: > // ... Note that f must ... Done. https://codereview.appspot.com/5502043/diff/12001/state/topology.go#newcode230 state/topology.go:230: changeContent := func(topologyYaml string, stat *zookeeper.Stat) (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > We know that within this function we're working to retryTopologyChange, so we > can trust on that context to use shorter variable names where the meaning is > clear, making the function more readable: > > s/changeContent/change/ > s/topologyYaml/yaml/ Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode15 state/unit.go:15: // and its subordinate parts. On 2012/01/10 13:02:17, niemeyer wrote: > // Unit represents the state of a service unit. Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode18 state/unit.go:18: id string On 2012/01/10 13:02:17, niemeyer wrote: > s/id/node/ Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode19 state/unit.go:19: serviceId string On 2012/01/10 13:02:17, niemeyer wrote: > s/serviceId/serviceNode/ Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode30 state/unit.go:30: func (u Unit) Id() string { On 2012/01/10 13:02:17, niemeyer wrote: > s/Id/zkNodeName/, and move to bottom next to the similar functions. Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode35 state/unit.go:35: // name and the sequence number. On 2012/01/10 13:02:17, niemeyer wrote: > // Name returns the unit name. Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode41 state/unit.go:41: // any machine it's assigned to. On 2012/01/10 13:02:17, niemeyer wrote: > s/any machine/the machine/ Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode45 state/unit.go:45: return newError("service state has changed") On 2012/01/10 13:02:17, niemeyer wrote: > stateChanged Done. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode49 state/unit.go:49: // have to deal with conflicts. On 2012/01/10 13:02:17, niemeyer wrote: > This sounds sensible, but I don't think it's what the logic below is doing. > Please fix it below and watch out for those cases. Done. Ooops, indeed, entered the standard error checking too fast. https://codereview.appspot.com/5502043/diff/12001/state/unit.go#newcode75 state/unit.go:75: // sequence number. On 2012/01/10 13:02:17, niemeyer wrote: > You're adding unnecessary typing information to the comment. > Please change the function declaration to this: > > parseUnitName(name string) (serviceName string, seqNo int, err error) > > And use this comment: > > // parseUnitName parses a unit name like "wordpress/0" into > // its service name and sequence number parts. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go File state/util.go (right): https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode14 state/util.go:14: // newError allows a quick error creation with arguments. On 2012/01/10 13:02:17, niemeyer wrote: > Drop this function and use fmt.Errorf. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode20 state/util.go:20: // based on a path. On 2012/01/10 13:02:17, niemeyer wrote: > s/zkStringMap/zkMap/, for the reason stated below, and use the following > comment: > > // zkMap reads the yaml data of path from zk and > // parses it into a map. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode21 state/util.go:21: func zkStringMap(zk *zookeeper.Conn, path string) (map[string]string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > The map type here is a problem. yaml has typing information, so if you unmarshal > an int, it should be marshalled back as an int rather than as a string. > > Make this map: map[string]interface{}, and do the casting as appropriate > (checking for errors!). Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode22 state/util.go:22: // Fetch raw data. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode27 state/util.go:27: // Unmarshal it. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode35 state/util.go:35: // zkStringMapField returns a field our of a string map returned by stringMap(). On 2012/01/10 13:02:17, niemeyer wrote: > Turn this into zkMapField, per the logic above, and fix the comment: > > // zkMapField reads path from zk as a yaml map, and returns > // the value for field. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode36 state/util.go:36: func zkStringMapField(zk *zookeeper.Conn, path, field string) (string, error) { On 2012/01/10 13:02:17, niemeyer wrote: > Use named results: > > (value interface{}, err error) Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode37 state/util.go:37: // Get the map. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode42 state/util.go:42: // Look if field exists. On 2012/01/10 13:02:17, niemeyer wrote: > Drop comment. Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode45 state/util.go:45: return "", newError("cannot find field '%s' in path '%s'", field, path) On 2012/01/10 13:02:17, niemeyer wrote: > fmt.Errorf > s/'%s'/%q/g Done. https://codereview.appspot.com/5502043/diff/12001/state/util.go#newcode52 state/util.go:52: // First recursively delete the cildren. On 2012/01/10 13:02:17, niemeyer wrote: > Good comments! > > Except for a minor typo: > > s/cildren/children/ Done.
Sign in to reply to this message.
One more small pass. https://codereview.appspot.com/5502043/diff/14002/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/14002/state/service.go#newcode36 state/service.go:36: return "", fmt.Errorf("charm has illegal type") Imagine yourself in the console looking at this error message. What would you understand from it? https://codereview.appspot.com/5502043/diff/14002/state/service.go#newcode148 state/service.go:148: // UnitNames returns the names of all units of service. Sorry, this was my mistake. s/service/s/, please. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode104 state/state_test.go:104: // Open state. !Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode118 state/state_test.go:118: c.Assert(err, ErrorMatches, "service with name \"pressword\" cannot be found") Use `` for the string, so that you can avoid the \" escaping. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode157 state/state_test.go:157: c.Assert(err, ErrorMatches, "\"wordpress\" is no valid unit name") Ditto. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode159 state/state_test.go:159: c.Assert(err, ErrorMatches, "\"wordpress/0/0\" is no valid unit name") Ditto. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode161 state/state_test.go:161: c.Assert(err, ErrorMatches, "can't find unit \"pressword/0\" on service \"wordpress\"") Ditto. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode171 state/state_test.go:171: c.Assert(len(unitNames), Equals, 2) Why was this changed? This isn't testing the feature at all now. The previous version was good. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode176 state/state_test.go:176: c.Assert(len(units), Equals, 2) This is bad too and I missed it before. Check what are the units retrieved, otherwise the logic may be completely bogus. https://codereview.appspot.com/5502043/diff/14002/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/14002/state/topology.go#newcode76 state/topology.go:76: func (t *topology) serviceNodeName(name string) (string, error) { This is inconsistent as it is the only method name that refers to NodeName rather than Node, even though all of them talk about node names.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/5502043/diff/14002/state/service.go File state/service.go (right): https://codereview.appspot.com/5502043/diff/14002/state/service.go#newcode36 state/service.go:36: return "", fmt.Errorf("charm has illegal type") On 2012/01/11 11:42:50, niemeyer wrote: > Imagine yourself in the console looking at this error message. What would you > understand from it? Done. https://codereview.appspot.com/5502043/diff/14002/state/service.go#newcode148 state/service.go:148: // UnitNames returns the names of all units of service. On 2012/01/11 11:42:50, niemeyer wrote: > Sorry, this was my mistake. s/service/s/, please. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode104 state/state_test.go:104: // Open state. On 2012/01/11 11:42:50, niemeyer wrote: > !Done. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode118 state/state_test.go:118: c.Assert(err, ErrorMatches, "service with name \"pressword\" cannot be found") On 2012/01/11 11:42:50, niemeyer wrote: > Use `` for the string, so that you can avoid the \" escaping. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode157 state/state_test.go:157: c.Assert(err, ErrorMatches, "\"wordpress\" is no valid unit name") On 2012/01/11 11:42:50, niemeyer wrote: > Ditto. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode159 state/state_test.go:159: c.Assert(err, ErrorMatches, "\"wordpress/0/0\" is no valid unit name") On 2012/01/11 11:42:50, niemeyer wrote: > Ditto. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode161 state/state_test.go:161: c.Assert(err, ErrorMatches, "can't find unit \"pressword/0\" on service \"wordpress\"") On 2012/01/11 11:42:50, niemeyer wrote: > Ditto. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode171 state/state_test.go:171: c.Assert(len(unitNames), Equals, 2) On 2012/01/11 11:42:50, niemeyer wrote: > Why was this changed? This isn't testing the feature at all now. The previous > version was good. Done. https://codereview.appspot.com/5502043/diff/14002/state/state_test.go#newcode176 state/state_test.go:176: c.Assert(len(units), Equals, 2) On 2012/01/11 11:42:50, niemeyer wrote: > This is bad too and I missed it before. Check what are the units retrieved, > otherwise the logic may be completely bogus. Done. https://codereview.appspot.com/5502043/diff/14002/state/topology.go File state/topology.go (right): https://codereview.appspot.com/5502043/diff/14002/state/topology.go#newcode76 state/topology.go:76: func (t *topology) serviceNodeName(name string) (string, error) { On 2012/01/11 11:42:50, niemeyer wrote: > This is inconsistent as it is the only method name that refers to NodeName > rather than Node, even though all of them talk about node names. Done.
Sign in to reply to this message.
LGTM! Thanks for the great work.
Sign in to reply to this message.
*** Submitted: Initial adding of the state model to the Go port of juju As a first step in adding the Go port the state can be opened and first operations ond Service and Unit can be done. Also needed first topology functionality is part of this branch. R= CC= https://codereview.appspot.com/5502043
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
