worker: Introducing StringsWorker

worker: Introducing StringsWorker

Implemented StringsWorker after the model
of NotifyWorker - waiting for changes from
api.StringsWatcher and calling the handler.

It was necessary to do some refactoring to
NotifyWorker and tests to avoid duplication.

Also improved tests.

https://code.launchpad.net/~dimitern/juju-core/084-stringsworker/+merge/178032

(do not edit description out of merge proposal)

[dimitern, 2013-08-01 10:22:32]

Please take a look.

[mue, 2013-08-01 10:35:24]

LGTM, nice, only minor comments.

https://codereview.appspot.com/12198044/diff/1/worker/interface.go
File worker/interface.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/interface.go#newcode11
worker/interface.go:11: // Wait for the worker to finish what it is doing an
exit
at exit? or on exit?

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode94
worker/notifyworker.go:94: // Tear down the handler, but ensure any error is
propagated
Why the comment in the method?

[rog, 2013-08-01 11:18:47]

Looks good, but I think there are some further simplifications that can be made.
See below.

https://codereview.appspot.com/12198044/diff/1/worker/interface.go
File worker/interface.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/interface.go#newcode10
worker/interface.go:10: type CommonWorker interface {

I don't think this interface is necessary.
The already-existing Worker interface defines Kill and Wait,
which is all that's necessary for a worker.

Stop is a convenience, and there's already a Stop function that builds on top of
the Worker type.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go
File worker/notifyworker.go (left):

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#oldcode134
worker/notifyworker.go:134: panic("unreachable")
yay!

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode25
worker/notifyworker.go:25: closedHandler func(watcher.Errer) error
can we just call this mustErr, as that represents the
intent of the code?

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode32
worker/notifyworker.go:32: type NotifyWorker CommonWorker
I don't think we need this type.
Let's just make NewNotifyWorker return a Worker.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode54
worker/notifyworker.go:54: String() string
I don't think we need a String method at all. It was used in the
old runTasks logic in cmd/jujud, but nothing calls
String on a worker now.

Please delete it from all relevant places - it's just clutter.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode60
worker/notifyworker.go:60: func NewNotifyWorker(handler NotifyWatchHandler)
NotifyWorker {
I think this should return a Worker.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode78
worker/notifyworker.go:78: func (nw *notifyWorker) Stop() error {
I don't think we need to bother implementing this method.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode93
worker/notifyworker.go:93: func notifyHandlerTearDown(handler
NotifyWatchHandler, t *tomb.Tomb) {
how about this?

func propagateTearDown(handler interface{TearDown() error}, t *tomb.Tomb) {
    if err := handler.TearDown(); err != nil {
         t.Kill(err)
    }
}

then you won't need two differently named functions.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode103
worker/notifyworker.go:103: defer notifyHandlerTearDown(nw.handler, &nw.tomb)
I'm not convinced that it's right to call TearDown even if SetUp has returned an
error. The current logic also means that if there's a set up and then a tear
down error, only the tear down error will be seen, which seems wrong to me.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode106
worker/notifyworker.go:106: // We don't bother to propogate an error, because we
s/propo/propa/

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go
File worker/stringsworker.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode23
worker/stringsworker.go:23: closedHandler func(watcher.Errer) error
s/closedHandler/mustErr/

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode51
worker/stringsworker.go:51: String() string
d

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode57
worker/stringsworker.go:57: func NewStringsWorker(handler StringsWatchHandler)
StringsWorker {
I think this should return a Worker.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode75
worker/stringsworker.go:75: func (sw *stringsWorker) Stop() error {
d

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode87
worker/stringsworker.go:87: func (sw *stringsWorker) String() string {
d

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode91
worker/stringsworker.go:91: func stringsHandlerTearDown(handler
StringsWatchHandler, t *tomb.Tomb) {
d (given a generic version elsewhere)

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcod...
worker/stringsworker.go:100: var err error
Rather than explicitly declaring these, how about:

w, err := sw.handler.SetUp()
if err != nil {
    etc
}

same applies in notifyworker.go

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker_test.go
File worker/stringsworker_test.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker_test.go#n...
worker/stringsworker_test.go:157: c.Errorf("Timeout changes triggering change
after %s", coretesting.LongWait)
"Timed out trying to trigger change" ?

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker_test.go#n...
worker/stringsworker_test.go:165: return
d

[dimitern, 2013-08-01 12:53:54]

Please take a look.

https://codereview.appspot.com/12198044/diff/1/worker/interface.go
File worker/interface.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/interface.go#newcode10
worker/interface.go:10: type CommonWorker interface {
On 2013/08/01 11:18:47, rog wrote:
> 
> I don't think this interface is necessary.
> The already-existing Worker interface defines Kill and Wait,
> which is all that's necessary for a worker.
> 
> Stop is a convenience, and there's already a Stop function that builds on top
of
> the Worker type.

Done.

https://codereview.appspot.com/12198044/diff/1/worker/interface.go#newcode11
worker/interface.go:11: // Wait for the worker to finish what it is doing an
exit
On 2013/08/01 10:35:24, mue wrote:
> at exit? or on exit?

"and exit", I think. I just copied that from before, but now it's gone, as Roger
suggested, to simplify things.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode25
worker/notifyworker.go:25: closedHandler func(watcher.Errer) error
On 2013/08/01 11:18:47, rog wrote:
> can we just call this mustErr, as that represents the
> intent of the code?

Done.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode32
worker/notifyworker.go:32: type NotifyWorker CommonWorker
On 2013/08/01 11:18:47, rog wrote:
> I don't think we need this type.
> Let's just make NewNotifyWorker return a Worker.

Done.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode54
worker/notifyworker.go:54: String() string
On 2013/08/01 11:18:47, rog wrote:
> I don't think we need a String method at all. It was used in the
> old runTasks logic in cmd/jujud, but nothing calls
> String on a worker now.
> 
> Please delete it from all relevant places - it's just clutter.

Done.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode60
worker/notifyworker.go:60: func NewNotifyWorker(handler NotifyWatchHandler)
NotifyWorker {
On 2013/08/01 11:18:47, rog wrote:
> I think this should return a Worker.

Done.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode78
worker/notifyworker.go:78: func (nw *notifyWorker) Stop() error {
On 2013/08/01 11:18:47, rog wrote:
> I don't think we need to bother implementing this method.

Done. As discussed, worker.Stop(x) can be used instead.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode93
worker/notifyworker.go:93: func notifyHandlerTearDown(handler
NotifyWatchHandler, t *tomb.Tomb) {
On 2013/08/01 11:18:47, rog wrote:
> how about this?
> 
> func propagateTearDown(handler interface{TearDown() error}, t *tomb.Tomb) {
>     if err := handler.TearDown(); err != nil {
>          t.Kill(err)
>     }
> }
> 
> then you won't need two differently named functions.

Done.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode94
worker/notifyworker.go:94: // Tear down the handler, but ensure any error is
propagated
On 2013/08/01 10:35:24, mue wrote:
> Why the comment in the method?

Because it was describing what's being done, rather than as a ill-formatted doc
comment, as before.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode103
worker/notifyworker.go:103: defer notifyHandlerTearDown(nw.handler, &nw.tomb)
On 2013/08/01 11:18:47, rog wrote:
> I'm not convinced that it's right to call TearDown even if SetUp has returned
an
> error. The current logic also means that if there's a set up and then a tear
> down error, only the tear down error will be seen, which seems wrong to me.

As I understand it, you propose to call this after SetUp and report any error
from it before.

https://codereview.appspot.com/12198044/diff/1/worker/notifyworker.go#newcode106
worker/notifyworker.go:106: // We don't bother to propogate an error, because we
On 2013/08/01 11:18:47, rog wrote:
> s/propo/propa/

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go
File worker/stringsworker.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode23
worker/stringsworker.go:23: closedHandler func(watcher.Errer) error
On 2013/08/01 11:18:47, rog wrote:
> s/closedHandler/mustErr/

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode51
worker/stringsworker.go:51: String() string
On 2013/08/01 11:18:47, rog wrote:
> d

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode57
worker/stringsworker.go:57: func NewStringsWorker(handler StringsWatchHandler)
StringsWorker {
On 2013/08/01 11:18:47, rog wrote:
> I think this should return a Worker.

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode75
worker/stringsworker.go:75: func (sw *stringsWorker) Stop() error {
On 2013/08/01 11:18:47, rog wrote:
> d

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode87
worker/stringsworker.go:87: func (sw *stringsWorker) String() string {
On 2013/08/01 11:18:47, rog wrote:
> d

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcode91
worker/stringsworker.go:91: func stringsHandlerTearDown(handler
StringsWatchHandler, t *tomb.Tomb) {
On 2013/08/01 11:18:47, rog wrote:
> d (given a generic version elsewhere)

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker.go#newcod...
worker/stringsworker.go:100: var err error
On 2013/08/01 11:18:47, rog wrote:
> Rather than explicitly declaring these, how about:
> 
> w, err := sw.handler.SetUp()
> if err != nil {
>     etc
> }
> 
> same applies in notifyworker.go

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker_test.go
File worker/stringsworker_test.go (right):

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker_test.go#n...
worker/stringsworker_test.go:157: c.Errorf("Timeout changes triggering change
after %s", coretesting.LongWait)
On 2013/08/01 11:18:47, rog wrote:
> "Timed out trying to trigger change" ?

Done.

https://codereview.appspot.com/12198044/diff/1/worker/stringsworker_test.go#n...
worker/stringsworker_test.go:165: return
On 2013/08/01 11:18:47, rog wrote:
> d

Done.

[dimitern, 2013-08-01 12:57:34]

Please take a look.

[dimitern, 2013-08-01 13:04:33]

Please take a look.

[rog, 2013-08-01 13:31:29]

On 2013/08/01 13:04:33, dimitern wrote:
> Please take a look.

LGTM, thanks!

[rog, 2013-08-01 13:31:36]

https://codereview.appspot.com/12198044/diff/24001/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/12198044/diff/24001/worker/notifyworker.go#new...
worker/notifyworker.go:72: // propagated tears down the handler, but ensures any
error is
s/propagated/propagateTearDown/

[dimitern, 2013-08-01 14:09:03]

Please take a look.

https://codereview.appspot.com/12198044/diff/24001/worker/notifyworker.go
File worker/notifyworker.go (right):

https://codereview.appspot.com/12198044/diff/24001/worker/notifyworker.go#new...
worker/notifyworker.go:72: // propagated tears down the handler, but ensures any
error is
On 2013/08/01 13:31:36, rog wrote:
> s/propagated/propagateTearDown/

Oops. Done.