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

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with updated doc comment and better tests.

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#newcode23
worker/resumer/resumer.go:23: // NewResumer ...
... ?

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)
to be consistent with the other workers, this error would be fatal.
any particular reason why it's not?

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)
how does this test that ResumeTransactions is actually called? (given
that it actually isn't in this test)

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

« Back to merge proposal