Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1533)

Issue 10978043: worker/notifyworker.go: Common worker logic

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by jameinel
Modified:
10 years, 8 months ago
Reviewers:
mue, dimitern, mp+173354, dfc, fwereade
Visibility:
Public.

Description

worker/notifyworker.go: Common worker logic While I was looking to implement Upgrader as a worker, I realized that there was a lot of bits necessary to implement a Worker. You have to integrate with a tomb, and monitor the worker, and handle a modest interface, etc. It isn't a terrible amount, but it turns out to be a lot of boilerplate that actually takes a lot if you want to test all the edge cases. This patch pulls all of that edge-case logic into a NotifyWorker helper. Essentially you implement a smaller api (WatchHandler), and hand that to the NotifyWorker. It then handles all the logic about shutting down cleanly, and you just integrate your couple of actions based on triggers. I have a follow up patch so you can see what the Cleaner worker looks like with this change. Probably the biggest advantage of this patch is that I have a very well tested edge-case-handling code, which can now just be re-used for all the other workers, and you don't have to worry about if your code is stopping the watcher it started, or if it is shutting down at the right time, etc. https://code.launchpad.net/~jameinel/juju-core/notify-worker/+merge/173354 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 23

Patch Set 2 : worker/notifywatcher.go: Common worker logic #

Total comments: 24

Patch Set 3 : worker/notifywatcher.go: Common worker logic #

Total comments: 13

Patch Set 4 : worker/notifyworker.go: Common worker logic #

Unified diffs Side-by-side diffs Delta from patch set Stats (+534 lines, -0 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M worker/export_test.go View 2 chunks +10 lines, -0 lines 0 comments Download
A worker/notifyworker.go View 1 2 3 1 chunk +136 lines, -0 lines 0 comments Download
A worker/notifyworker_test.go View 1 2 3 1 chunk +386 lines, -0 lines 0 comments Download

Messages

Total messages: 12
jameinel
Please take a look.
10 years, 8 months ago (2013-07-07 14:02:36 UTC) #1
jameinel
On 2013/07/07 14:02:36, jameinel wrote: > Please take a look. See https://codereview.appspot.com/10876048/ for an example ...
10 years, 8 months ago (2013-07-07 14:08:13 UTC) #2
dfc
https://codereview.appspot.com/10978043/diff/1/worker/export_test.go File worker/export_test.go (right): https://codereview.appspot.com/10978043/diff/1/worker/export_test.go#newcode19 worker/export_test.go:19: old := nw.closedHandler this will probably cause the race ...
10 years, 8 months ago (2013-07-08 02:46:07 UTC) #3
jameinel
Please take a look. https://codereview.appspot.com/10978043/diff/1/worker/export_test.go File worker/export_test.go (right): https://codereview.appspot.com/10978043/diff/1/worker/export_test.go#newcode19 worker/export_test.go:19: old := nw.closedHandler On 2013/07/08 ...
10 years, 8 months ago (2013-07-08 06:09:49 UTC) #4
mue
Very good approach, but would like some changes. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newcode30 worker/notifyworker.go:30: Wait() ...
10 years, 8 months ago (2013-07-08 09:22:39 UTC) #5
fwereade
Very nice. LGTM (although some of mue's comments should be handled too) https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go ...
10 years, 8 months ago (2013-07-08 09:53:48 UTC) #6
mue
https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newcode87 worker/notifyworker.go:87: // to debug. On 2013/07/08 09:53:49, fwereade wrote: > ...
10 years, 8 months ago (2013-07-08 10:05:35 UTC) #7
jameinel
Please take a look. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newcode38 worker/notifyworker.go:38: type WatchHandler interface { On ...
10 years, 8 months ago (2013-07-08 10:17:36 UTC) #8
fwereade
I'd generally be hoping that whatever might return a nil watcher would itself be unit ...
10 years, 8 months ago (2013-07-08 10:24:08 UTC) #9
mue
LGTM
10 years, 8 months ago (2013-07-08 10:30:58 UTC) #10
dimitern
Nice, LGTM with mostly trivial suggestions. There is some inconsistency with exported/unexported helpers in the ...
10 years, 8 months ago (2013-07-08 10:51:03 UTC) #11
jameinel
10 years, 8 months ago (2013-07-08 11:10:02 UTC) #12
Please take a look.

https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc...
worker/notifyworker.go:44: TearDown()
On 2013/07/08 10:24:08, fwereade wrote:
> On 2013/07/08 10:17:36, jameinel wrote:
> > On 2013/07/08 09:53:49, fwereade wrote:
> > > TearDown ought to be able to error, I think.
> > 
> > Certainly I initially wrote it returning an error, but found that everywhere
> > that was calling TearDown was unable to do anything with that error because
it
> > was already dealing with the current error.
> > 
> > Since the logic is going to end up "Log an error if it occurs during
cleanup"
> > I'd be happier to have people writing the cleanup function realize that
their
> > errors are going to be ignored.
> 
> The killing error might easily be nil. If we're going to just drop these on
the
> floor we may as well not bother using a tomb at all.

Done.

https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new...
worker/notifyworker.go:43: 
On 2013/07/08 10:51:04, dimitern wrote:
> d?

If you want spaces between them *I* find it easier to read each one with a blank
line before it. That way the block which is the description of the type is by
itself, and each block which describes the attribute is by itself.

I don't quite understand why the first block would be special and attached to
the type line.

https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.go
File worker/notifyworker_test.go (right):

https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g...
worker/notifyworker_test.go:13: 
On 2013/07/08 10:51:04, dimitern wrote:
> not getting the separation here

standard library imports (time sync fmt net/http, etc.)

3rd party imports (gocheck tomb mgo)

juju-core imports (juju-core/state, juju-core/worker, etc.)

https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g...
worker/notifyworker_test.go:50: type ActionsHandler struct {
On 2013/07/08 10:51:04, dimitern wrote:
> why exported?

It doesn't need to be, but *its a test suite* nothing can import it anyway.

I don't find the hiding of things to be particularly helpful in test suites, and
I still haven't gotten over "CamelCase" for ClassNames.

Privatized.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b