Merge lp:~themue/juju-core/026-transaction-resumer into lp:~go-bot/juju-core/trunk

[Frank Mueller]

resumer: periodically resume transaction

The resumer is a periodically worker which is
resuming pending transactions in state each
minute.

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

resumer: periodically resume transaction

The resumer is a periodically worker which is
resuming pending transactions in state each
minute.

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

[Frank Mueller (themue)]

Reviewers: mp+169234_code.launchpad.net,

Message:
Please take a look.

Description:
resumer: periodically resume transaction

The resumer is a periodically worker which is
resuming pending transactions in state each
minute.

https://code.launchpad.net/~themue/juju-core/026-transaction-resumer/+merge/169234

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10266043/

Affected files:
   A [revision details]
   M state/state.go
   A worker/resumer/resumer.go
   A worker/resumer/resumer_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130613115315-e58eqa122kc07o1a
+New revision: <email address hidden>

Index: state/state.go
=== modified file 'state/state.go'
--- state/state.go 2013-06-12 02:45:22 +0000
+++ state/state.go 2013-06-13 15:29:49 +0000
@@ -1123,6 +1123,11 @@
   return nil
  }

+// ResumeTransactions resumes all pending transactions.
+func (st *State) ResumeTransactions() error {
+ return st.runner.ResumeAll()
+}
+
  var tagPrefix = map[byte]string{
   'm': "machine-",
   's': "service-",

Index: worker/resumer/resumer.go
=== added file 'worker/resumer/resumer.go'
--- worker/resumer/resumer.go 1970-01-01 00:00:00 +0000
+++ worker/resumer/resumer.go 2013-06-13 15:29:49 +0000
@@ -0,0 +1,62 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package resumer
+
+import (
+ "fmt"
+ "launchpad.net/juju-core/log"
+ "launchpad.net/juju-core/state"
+ "launchpad.net/tomb"
+ "time"
+)
+
+// interval sets how often the resuming is called.
+const interval = time.Minute
+
+// Resumer is responsible for a periodical resuming of pending
transactions.
+type Resumer struct {
+ tomb tomb.Tomb
+ st *state.State
+}
+
+// NewResumer ...
+func NewResumer(st *state.State) *Resumer {
+ rr := &Resumer{st: st}
+ go func() {
+ defer rr.tomb.Done()
+ rr.tomb.Kill(rr.loop())
+ }()
+ return rr
+}
+
+func (rr *Resumer) String() string {
+ return fmt.Sprintf("resumer")
+}
+
+func (rr *Resumer) Kill() {
+ rr.tomb.Kill(nil)
+}
+
+func (rr *Resumer) Stop() error {
+ rr.tomb.Kill(nil)
+ return rr.tomb.Wait()
+}
+
+func (rr *Resumer) Wait() error {
+ return rr.tomb.Wait()
+}
+
+func (rr *Resumer) loop() error {
+ for {
+ select {
+ case <-rr.tomb.Dying():
+ return tomb.ErrDying
+ case <-time.After(interval):
+ if err := rr.st.ResumeTransactions(); err != nil {
+ log.Errorf("worker/resumer: cannot resume transactions: %v", err)
+ }
+ }
+ }
+ panic("unreachable")
+}

Index: worker/resumer/resumer_test.go
=== added file 'worker/resumer/resumer_test.go'
--- worker/resumer/resumer_test.go 1970-01-01 00:00:00 +0000
+++ worker/resumer/resumer_test.go 2013-06-13 15:29:49 +0000
@@ -0,0 +1,28 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package resumer_test
+
+import (
+ . "launchpad.net/gocheck"
+ "launchpad.net/juju-core/juju/testing"
+ coretesting "launchpad.net/juju-core/testing"
+ "launchpad.net/juju-c...

[William Reade (fwereade)]

LGTM

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

[Roger Peppe (rogpeppe)]

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/

[William Reade (fwereade)]

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/

[Frank Mueller (themue)]

Please take a look.

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 ...
On 2013/06/14 15:46:07, rog wrote:
> ... ?

Done.

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

[William Reade (fwereade)]

LGTM

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

=== modified file 'state/state.go'
--- state/state.go	2013-06-18 19:49:00 +0000
+++ state/state.go	2013-06-19 11:55:35 +0000
@@ -1203,6 +1203,11 @@
 	return nil
 }
 
+// ResumeTransactions resumes all pending transactions.
+func (st *State) ResumeTransactions() error {
+	return st.runner.ResumeAll()
+}
+
 var tagPrefix = map[byte]string{
 	'm': "machine-",
 	's': "service-",

=== added directory 'worker/resumer'
=== added file 'worker/resumer/export_test.go'
--- worker/resumer/export_test.go	1970-01-01 00:00:00 +0000
+++ worker/resumer/export_test.go	2013-06-19 11:55:35 +0000
@@ -0,0 +1,16 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package resumer
+
+import (
+	"time"
+)
+
+func SetInterval(i time.Duration) {
+	interval = i
+}
+
+func RestoreInterval() {
+	interval = defaultInterval
+}

=== added file 'worker/resumer/resumer.go'
--- worker/resumer/resumer.go	1970-01-01 00:00:00 +0000
+++ worker/resumer/resumer.go	2013-06-19 11:55:35 +0000
@@ -0,0 +1,71 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package resumer
+
+import (
+	"fmt"
+	"launchpad.net/juju-core/log"
+	"launchpad.net/tomb"
+	"time"
+)
+
+// defaultInterval is the standard value for the interval setting.
+const defaultInterval = time.Minute
+
+// interval sets how often the resuming is called.
+var interval = defaultInterval
+
+// TransactionResumer defines the interface for types capable to
+// resume transactions.
+type TransactionResumer interface {
+	// ResumeTransactions resumes all pending transactions.
+	ResumeTransactions() error
+}
+
+// Resumer is responsible for a periodical resuming of pending transactions.
+type Resumer struct {
+	tomb tomb.Tomb
+	tr   TransactionResumer
+}
+
+// NewResumer periodically resumes pending transactions.
+func NewResumer(tr TransactionResumer) *Resumer {
+	rr := &Resumer{tr: tr}
+	go func() {
+		defer rr.tomb.Done()
+		rr.tomb.Kill(rr.loop())
+	}()
+	return rr
+}
+
+func (rr *Resumer) String() string {
+	return fmt.Sprintf("resumer")
+}
+
+func (rr *Resumer) Kill() {
+	rr.tomb.Kill(nil)
+}
+
+func (rr *Resumer) Stop() error {
+	rr.tomb.Kill(nil)
+	return rr.tomb.Wait()
+}
+
+func (rr *Resumer) Wait() error {
+	return rr.tomb.Wait()
+}
+
+func (rr *Resumer) loop() error {
+	for {
+		select {
+		case <-rr.tomb.Dying():
+			return tomb.ErrDying
+		case <-time.After(interval):
+			if err := rr.tr.ResumeTransactions(); err != nil {
+				log.Errorf("worker/resumer: cannot resume transactions: %v", err)
+			}
+		}
+	}
+	panic("unreachable")
+}

=== added file 'worker/resumer/resumer_test.go'
--- worker/resumer/resumer_test.go	1970-01-01 00:00:00 +0000
+++ worker/resumer/resumer_test.go	2013-06-19 11:55:35 +0000
@@ -0,0 +1,57 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package resumer_test
+
+import (
+	stdtesting "testing"
+	"time"
+
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/juju/testing"
+	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/worker/resumer"
+)
+
+func TestPackage(t *stdtesting.T) {
+	coretesting.MgoTestPackage(t)
+}
+
+type ResumerSuite struct {
+	testing.JujuConnSuite
+}
+
+var _ = Suite(&ResumerSuite{})
+
+func (s *ResumerSuite) TestRunStopWithState(c *C) {
+	// Test with state ensures that state fulfills the 
+	// TransactionResumer interface.
+	rr := resumer.NewResumer(s.State)
+
+	c.Assert(rr.Stop(), IsNil)
+}
+
+func (s *ResumerSuite) TestResumerCalls(c *C) {
+	// Shorter interval and mock help to count
+	// the resumer calls in a given timespan.
+	resumer.SetInterval(10 * time.Millisecond)
+	defer resumer.RestoreInterval()
+
+	tr := &transactionResumerMock{0}
+	rr := resumer.NewResumer(tr)
+	defer func() { c.Assert(rr.Stop(), IsNil) }()
+
+	time.Sleep(55 * time.Millisecond)
+	c.Assert(tr.counter, Equals, 5)
+}
+
+// transactionResumerMock is used to check the
+// calls of ResumeTransactions().
+type transactionResumerMock struct {
+	counter int
+}
+
+func (t *transactionResumerMock) ResumeTransactions() error {
+	t.counter++
+	return nil
+}