Code review comment for lp:~themue/juju-core/026-transaction-resumer

Revision history for this message
William Reade (fwereade) wrote :

quick responses to rog's comments

https://codereview.appspot.com/10266043/diff/1/worker/resumer/resumer.go
File worker/resumer/resumer.go (right):

https://codereview.appspot.com/10266043/diff/1/worker/resumer/resumer.go#newcode57
worker/resumer/resumer.go:57: log.Errorf("worker/resumer: cannot resume
transactions: %v", err)
On 2013/06/14 15:46:07, rog wrote:
> to be consistent with the other workers, this error would be fatal.
> any particular reason why it's not?

I don't think an error is important enough to justify retrying so much
sooner.

https://codereview.appspot.com/10266043/diff/1/worker/resumer/resumer_test.go
File worker/resumer/resumer_test.go (right):

https://codereview.appspot.com/10266043/diff/1/worker/resumer/resumer_test.go#newcode25
worker/resumer/resumer_test.go:25: rr := resumer.NewResumer(s.State)
On 2013/06/14 15:46:07, rog wrote:
> how does this test that ResumeTransactions is actually called? (given
that it
> actually isn't in this test)

Ha. We should ofc use `type Resumer interface { ResumeTransactions()
error}` and check at least that a suitable method gets called on a mock.

https://codereview.appspot.com/10266043/

« Back to merge proposal