Issue 6059047: Added ResolvedWatcher for Units. (Closed)
DescriptionAdded ResolvedWatcher for Units.
It reads the resolved mode out of the node content if
it's created or changed.
https://code.launchpad.net/~themue/juju/go-state-unit-resolved-watcher/+merge/102497
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 38
Patch Set 2 : Added ResolvedWatcher for Units. #Patch Set 3 : Added ResolvedWatcher for Units. #
MessagesTotal messages: 14
Please take a look.
Sign in to reply to this message.
This needs a pre-req branch when you're proposing. Look at the diffs. It's piling up every single branch you've sent before.
Sign in to reply to this message.
Looks generally sensible, only showstopper is upgrade node content. This isn't your fault -- I don't remember seeing this change discussed on the list -- but it needs to be done all the same. https://codereview.appspot.com/6059047/diff/1/state/state.go File state/state.go (right): https://codereview.appspot.com/6059047/diff/1/state/state.go#newcode292 state/state.go:292: // A non-existing node is treaten as an empty node. s/non-existing/nonexistent/ s/treaten/treated/ (I actually rather like "treaten", but... ;)) https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (left): https://codereview.appspot.com/6059047/diff/1/state/unit.go#oldcode453 state/unit.go:453: This file is getting a bit unwieldy IMO. Could we move the Watcher types into their own file? https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode287 state/unit.go:287: if err := validResolvedMode(mode, false); err != nil { I *think* acceptNone should be true here, shouldn't it? https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode469 state/unit.go:469: // parseResolveMode parses a given YAML for the resolve mode s/parseResolveMode/parseResolvedMode/ https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472 state/unit.go:472: setting := &struct{ Retry ResolvedMode }{} I have a slight preference for a non-anonymous struct (ResolvedNodeContent, or something) here... we need to set the content in the tests, and we will also need to do so from the CLI, and I'd rather have a reasonable certainty that everything hitting this node is using the same mechanism to do so. https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538 state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() { Maybe "run" rather than "loop"? https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode552 state/unit.go:552: case w.changeChan <- change.Exists: Existence isn't enough; there's a "force" upgrade mode now as well (meaning "upgrade even if not actually running"), and we'll need to handle that for feature parity. https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode600 state/unit.go:600: func (w *ResolvedWatcher) loop() { As above, I prefer "run"; ymmv. https://codereview.appspot.com/6059047/diff/1/state/watch_test.go File state/watch_test.go (right): https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode17 state/watch_test.go:17: return nil, false, false How can we hit this path? https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode77 state/watch_test.go:77: case <-time.After(200 * time.Millisecond): Why 200ms here and 100ms above? https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode119 state/watch_test.go:119: go func() { Given that the events *can* coalesce, even if it's not very likely in this instance, I think it would be better to interleave this stuff with the tests. https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode164 state/watch_test.go:164: go func() { As above. https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go File state/watcher/watcher.go (left): https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go#oldcode98 state/watcher/watcher.go:98: case w.changeChan <- w.content: I presume this is idiomatic? It surprised me a little, but it seems to make sense, so: nice :). https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go#newcode12 state/watcher/watcher.go:12: type ContentChange struct { Nice. https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:108: // so call GetW() with new loop run again. "so just try again", perhaps? https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher_test.go File state/watcher/watcher_test.go (left): https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher_test.go#o... state/watcher/watcher_test.go:55: go func() { Same event-coalescing concerns as in watch_test https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher_test.go#o... state/watcher/watcher_test.go:97: go func() { Ditto
Sign in to reply to this message.
https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472 state/unit.go:472: setting := &struct{ Retry ResolvedMode }{} On 2012/04/19 08:29:50, fwereade wrote: > I have a slight preference for a non-anonymous struct (ResolvedNodeContent, or > something) here... we need to set the content in the tests, and we will also > need to do so from the CLI, and I'd rather have a reasonable certainty that > everything hitting this node is using the same mechanism to do so. orthogonal to this, declaring as a bare variable and using & in the Unmarshal would read better IMHO, var setting struct { Retry ResolvedMode } if err := goyaml.Unmarshal([]byte(yaml), &setting); err != nil { https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538 state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() { On 2012/04/19 08:29:50, fwereade wrote: > Maybe "run" rather than "loop"? +1 ("loop" implies a particular implementation strategy which may not be used). https://codereview.appspot.com/6059047/diff/1/state/watch_test.go File state/watch_test.go (right): https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode17 state/watch_test.go:17: return nil, false, false On 2012/04/19 08:29:50, fwereade wrote: > How can we hit this path? +1. panic("not reached") https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode119 state/watch_test.go:119: go func() { On 2012/04/19 08:29:50, fwereade wrote: > Given that the events *can* coalesce, even if it's not very likely in this > instance, I think it would be better to interleave this stuff with the tests. +1. especially as it makes the tests run slightly faster.
Sign in to reply to this message.
Still waiting for the pre-req to be sorted out, but a few comments-on-comments. https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472 state/unit.go:472: setting := &struct{ Retry ResolvedMode }{} On 2012/04/19 08:29:50, fwereade wrote: > I have a slight preference for a non-anonymous struct (ResolvedNodeContent, or > something) here... we need to set the content in the tests, and we will also > need to do so from the CLI, and I'd rather have a reasonable certainty that > everything hitting this node is using the same mechanism to do so. -1. The API from this package isn't about ZooKeeper nodes. It's about services and machines. https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538 state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() { On 2012/04/19 08:58:49, rog wrote: > On 2012/04/19 08:29:50, fwereade wrote: > > Maybe "run" rather than "loop"? > > +1 ("loop" implies a particular implementation strategy which may not be used). -1. This *is* a loop, and this isn't part of the API, which means it *is* the implementation as well. I don't see the point.
Sign in to reply to this message.
https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472 state/unit.go:472: setting := &struct{ Retry ResolvedMode }{} On 2012/04/19 09:11:55, niemeyer wrote: > On 2012/04/19 08:29:50, fwereade wrote: > > I have a slight preference for a non-anonymous struct (ResolvedNodeContent, or > > something) here... we need to set the content in the tests, and we will also > > need to do so from the CLI, and I'd rather have a reasonable certainty that > > everything hitting this node is using the same mechanism to do so. > > -1. The API from this package isn't about ZooKeeper nodes. It's about services > and machines. It doesn't have to be public; how would you feel about "resolvedNodeContent" instead? I'm really just trying to extend the benefits of static typing to node content...
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6059047/diff/1/state/state.go File state/state.go (right): https://codereview.appspot.com/6059047/diff/1/state/state.go#newcode292 state/state.go:292: // A non-existing node is treaten as an empty node. On 2012/04/19 08:29:50, fwereade wrote: > s/non-existing/nonexistent/ > s/treaten/treated/ > > (I actually rather like "treaten", but... ;)) Done. https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (left): https://codereview.appspot.com/6059047/diff/1/state/unit.go#oldcode453 state/unit.go:453: On 2012/04/19 08:29:50, fwereade wrote: > This file is getting a bit unwieldy IMO. Could we move the Watcher types into > their own file? Yes, will do so after all watchers are submitted in an extra change. https://codereview.appspot.com/6059047/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode287 state/unit.go:287: if err := validResolvedMode(mode, false); err != nil { On 2012/04/19 08:29:50, fwereade wrote: > I *think* acceptNone should be true here, shouldn't it? Before I've extended validResolvedMode() for usage in the parsing function too it only accepted ResolvedRetryHooks and ResolvedNoHooks, but not ResolvedNone. https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode469 state/unit.go:469: // parseResolveMode parses a given YAML for the resolve mode On 2012/04/19 08:29:50, fwereade wrote: > s/parseResolveMode/parseResolvedMode/ Done. https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472 state/unit.go:472: setting := &struct{ Retry ResolvedMode }{} On 2012/04/19 08:58:49, rog wrote: > On 2012/04/19 08:29:50, fwereade wrote: > > I have a slight preference for a non-anonymous struct (ResolvedNodeContent, or > > something) here... we need to set the content in the tests, and we will also > > need to do so from the CLI, and I'd rather have a reasonable certainty that > > everything hitting this node is using the same mechanism to do so. > > orthogonal to this, declaring as a bare variable and using & in the Unmarshal > would read better IMHO, > var setting struct { > Retry ResolvedMode > } > if err := goyaml.Unmarshal([]byte(yaml), &setting); err != nil { Done. https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538 state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() { On 2012/04/19 09:11:55, niemeyer wrote: > On 2012/04/19 08:58:49, rog wrote: > > On 2012/04/19 08:29:50, fwereade wrote: > > > Maybe "run" rather than "loop"? > > > > +1 ("loop" implies a particular implementation strategy which may not be > used). > > -1. This *is* a loop, and this isn't part of the API, which means it *is* the > implementation as well. I don't see the point. Also prefer loop() due to the most important internal part: a change receiving loop. In my private projects I alternatively call it backend(). https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode600 state/unit.go:600: func (w *ResolvedWatcher) loop() { On 2012/04/19 08:29:50, fwereade wrote: > As above, I prefer "run"; ymmv. See above. https://codereview.appspot.com/6059047/diff/1/state/watch_test.go File state/watch_test.go (right): https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode17 state/watch_test.go:17: return nil, false, false On 2012/04/19 08:58:49, rog wrote: > On 2012/04/19 08:29:50, fwereade wrote: > > How can we hit this path? > > +1. > panic("not reached") All tests are refactored. https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode77 state/watch_test.go:77: case <-time.After(200 * time.Millisecond): On 2012/04/19 08:29:50, fwereade wrote: > Why 200ms here and 100ms above? Done. https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode119 state/watch_test.go:119: go func() { On 2012/04/19 08:58:49, rog wrote: > On 2012/04/19 08:29:50, fwereade wrote: > > Given that the events *can* coalesce, even if it's not very likely in this > > instance, I think it would be better to interleave this stuff with the tests. > > +1. especially as it makes the tests run slightly faster. Tests are refactored to be table-driven. https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode164 state/watch_test.go:164: go func() { On 2012/04/19 08:29:50, fwereade wrote: > As above. See above, tests are refactored. https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:108: // so call GetW() with new loop run again. On 2012/04/19 08:29:50, fwereade wrote: > "so just try again", perhaps? Sounds better, but no more watcher package changes in this proposal. https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher_test.go File state/watcher/watcher_test.go (left): https://codereview.appspot.com/6059047/diff/1/state/watcher/watcher_test.go#o... state/watcher/watcher_test.go:55: go func() { On 2012/04/19 08:29:50, fwereade wrote: > Same event-coalescing concerns as in watch_test Tests are refactored.
Sign in to reply to this message.
Frank, please have a look at the CL. It *still* contains unrelated changes that are not supposed to be reviewed here. https://codereview.appspot.com/6059047/diff/1/state/state.go File state/state.go (right): https://codereview.appspot.com/6059047/diff/1/state/state.go#newcode292 state/state.go:292: // A non-existing node is treaten as an empty node. On 2012/04/19 08:29:50, fwereade wrote: > s/non-existing/nonexistent/ The correct form is actually non-existent, AFAIK: http://oxforddictionaries.com/definition/non-existent
Sign in to reply to this message.
On 2012/04/23 23:09:23, niemeyer wrote: > Frank, please have a look at the CL. It *still* contains unrelated changes that > are not supposed to be reviewed here. > > https://codereview.appspot.com/6059047/diff/1/state/state.go > File state/state.go (right): > > https://codereview.appspot.com/6059047/diff/1/state/state.go#newcode292 > state/state.go:292: // A non-existing node is treaten as an empty node. > On 2012/04/19 08:29:50, fwereade wrote: > > s/non-existing/nonexistent/ > > The correct form is actually non-existent, AFAIK: > > http://oxforddictionaries.com/definition/non-existent A review comment of William for this proposal has been that the handling of "needs upgrade" is outdated due to a change of the Python code. Do I understand you right that you now want me to roll the changes back to the old implementation and create an extra change for it?
Sign in to reply to this message.
On 2012/04/24 10:01:51, TheMue wrote: > A review comment of William for this proposal has been that the handling of > "needs upgrade" is outdated due to a change of the Python code. Do I understand > you right that you now want me to roll the changes back to the old > implementation and create an extra change for it? Oh, so an unrelated change to the needs-upgrade functionality was merged in purposefully? This is still the same topic we talked about back at the Rally in Budapest: changes should be self-contained. Fixing the NeedsUpgrades behavior is definitely not part of what this CL is supposed to be covering. From its summary: """ Added ResolvedWatcher for Units. It reads the resolved mode out of the node content if it's created or changed. """ If you get a review comment from someone suggesting unrelated changes and you agree with them, just put that in another branch and move on.
Sign in to reply to this message.
On 2012/04/24 12:04:00, niemeyer wrote: > On 2012/04/24 10:01:51, TheMue wrote: > > A review comment of William for this proposal has been that the handling of > > "needs upgrade" is outdated due to a change of the Python code. Do I > understand > > you right that you now want me to roll the changes back to the old > > implementation and create an extra change for it? > > Oh, so an unrelated change to the needs-upgrade functionality was merged in > purposefully? > > This is still the same topic we talked about back at the Rally in Budapest: > changes should be self-contained. Fixing the NeedsUpgrades behavior is > definitely not part of what this CL is supposed to be covering. From its > summary: > > """ > Added ResolvedWatcher for Units. > > It reads the resolved mode out of the node content if > it's created or changed. > """ > > If you get a review comment from someone suggesting unrelated changes and you > agree with them, just put that in another branch and move on. Understood, I will try to be more thoughtful in future. So far to me the whole watcher topic has been one task. How about this one here? I would like you to review this time both changes at once as a final exception.
Sign in to reply to this message.
On 2012/04/24 12:10:21, TheMue wrote: > Understood, I will try to be more thoughtful in future. So far to me the whole > watcher topic has been one task. > > How about this one here? I would like you to review this time both changes at > once as a final exception. Sorry, but that was the same thing you said back in Budapest in that same circumstance. It should be trivial to revert this, given that you just added revisions on top of the original branch. Just follow something like this: bzr log # find the revision you started adding unrelated changes cd .. mv original-branch new-work bzr branch -r <revision> new-work original-branch cd original-branch lbox propose If done right, the above procedure should revert the changes to this CL in a painless way.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
I believe this became https://codereview.appspot.com/6120045/
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
