juju-core

Merge lp:~jameinel/juju-core/api-named-resources into lp:~go-bot/juju-core/trunk

  1. api-named-resources
  2. Merge into trunk
Proposed by John A Meinel
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: 2753
Proposed branch: lp:~jameinel/juju-core/api-named-resources
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 241 lines (+116/-13)
6 files modified
state/apiserver/admin.go (+2/-1)
state/apiserver/common/resource.go (+21/-0)
state/apiserver/common/resource_test.go (+50/-0)
state/apiserver/pinger_test.go (+19/-0)
state/apiserver/root.go (+8/-11)
state/apiserver/root_test.go (+16/-1)
To merge this branch: bzr merge lp:~jameinel/juju-core/api-named-resources
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+220208@code.launchpad.net

Commit message

state/apiserver/common: Resources.RegisterNamed

Two small changes here.

1) Change common.Resources to allow you to RegisterNamed rather than
   only allowing you to register and get back the Id. This was necessary
   for the second step, but generally allows us to declare Resources
   that Facades can know about that aren't reported to clients.

2) Change the pingTimeout object from being an attribute of srvRoot into
   being just another resource like the Watchers at a specific resource
   name.

While working on the changes for pingTimeout, I noticed that no tests
actually failed if I started returning nullTimeout. So I added a test
that ensures calling Ping() actually does delay the connection from
being closed. (We only had a test that if you never called Ping in time,
it would die, but not that Ping can delay that death.)

This is a step towards making all of the Facades follow a common
pattern, as this allows Pinger to also just take Resources rather than
needing to know about details of the srvRoot object.

https://codereview.appspot.com/93500044/

Description of the change

state/apiserver/common: Resources.RegisterNamed

Two small changes here.

1) Change common.Resources to allow you to RegisterNamed rather than
   only allowing you to register and get back the Id. This was necessary
   for the second step, but generally allows us to declare Resources
   that Facades can know about that aren't reported to clients.

2) Change the pingTimeout object from being an attribute of srvRoot into
   being just another resource like the Watchers at a specific resource
   name.

While working on the changes for pingTimeout, I noticed that no tests
actually failed if I started returning nullTimeout. So I added a test
that ensures calling Ping() actually does delay the connection from
being closed. (We only had a test that if you never called Ping in time,
it would die, but not that Ping can delay that death.)

This is a step towards making all of the Facades follow a common
pattern, as this allows Pinger to also just take Resources rather than
needing to know about details of the srvRoot object.

https://codereview.appspot.com/93500044/

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

Reviewers: mp+220208_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/common: Resources.RegisterNamed

Two small changes here.

1) Change common.Resources to allow you to RegisterNamed rather than
    only allowing you to register and get back the Id. This was necessary
    for the second step, but generally allows us to declare Resources
    that Facades can know about that aren't reported to clients.

2) Change the pingTimeout object from being an attribute of srvRoot into
    being just another resource like the Watchers at a specific resource
    name.

While working on the changes for pingTimeout, I noticed that no tests
actually failed if I started returning nullTimeout. So I added a test
that ensures calling Ping() actually does delay the connection from
being closed. (We only had a test that if you never called Ping in time,
it would die, but not that Ping can delay that death.)

This is a step towards making all of the Facades follow a common
pattern, as this allows Pinger to also just take Resources rather than
needing to know about details of the srvRoot object.

https://code.launchpad.net/~jameinel/juju-core/api-named-resources/+merge/220208

(do not edit description out of merge proposal)

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

Affected files (+118, -13 lines):
   A [revision details]
   M state/apiserver/admin.go
   M state/apiserver/common/resource.go
   M state/apiserver/common/resource_test.go
   M state/apiserver/pinger_test.go
   M state/apiserver/root.go
   M state/apiserver/root_test.go

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM

https://codereview.appspot.com/93500044/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'state/apiserver/admin.go'
--- state/apiserver/admin.go 2014-05-13 04:30:48 +0000
+++ state/apiserver/admin.go 2014-05-20 08:25:48 +0000
@@ -181,6 +181,7 @@
181 logger.Errorf("error closing the RPC connection: %v", err)181 logger.Errorf("error closing the RPC connection: %v", err)
182 }182 }
183 }183 }
184 newRoot.pingTimeout = newPingTimeout(action, maxClientPingInterval)184 pingTimeout := newPingTimeout(action, maxClientPingInterval)
185 newRoot.resources.RegisterNamed("pingTimeout", pingTimeout)
185 return nil186 return nil
186}187}
187188
=== modified file 'state/apiserver/common/resource.go'
--- state/apiserver/common/resource.go 2014-04-17 01:26:34 +0000
+++ state/apiserver/common/resource.go 2014-05-20 08:25:48 +0000
@@ -4,6 +4,7 @@
4package common4package common
55
6import (6import (
7 "fmt"
7 "strconv"8 "strconv"
8 "sync"9 "sync"
9)10)
@@ -50,6 +51,26 @@
50 return id51 return id
51}52}
5253
54// RegisterNamed registers the given resource. Callers must supply a unique
55// name for the given resource. It is an error to try to register another
56// resource with the same name as an already registered name. (This could be
57// softened that you can overwrite an existing one and it will be Stopped and
58// replaced, but we don't have a need for that yet.)
59// It is also an error to supply a name that is an integer string, since that
60// collides with the auto-naming from Register.
61func (rs *Resources) RegisterNamed(name string, r Resource) error {
62 rs.mu.Lock()
63 defer rs.mu.Unlock()
64 if _, err := strconv.Atoi(name); err == nil {
65 return fmt.Errorf("RegisterNamed does not allow integer names: %q", name)
66 }
67 if _, ok := rs.resources[name]; ok {
68 return fmt.Errorf("resource %q already registered", name)
69 }
70 rs.resources[name] = r
71 return nil
72}
73
53// Stop stops the resource with the given id and unregisters it.74// Stop stops the resource with the given id and unregisters it.
54// It returns any error from the underlying Stop call.75// It returns any error from the underlying Stop call.
55// It does not return an error if the resource has already76// It does not return an error if the resource has already
5677
=== modified file 'state/apiserver/common/resource_test.go'
--- state/apiserver/common/resource_test.go 2013-08-05 10:19:47 +0000
+++ state/apiserver/common/resource_test.go 2014-05-20 08:25:48 +0000
@@ -36,6 +36,50 @@
36 c.Assert(rs.Count(), gc.Equals, 2)36 c.Assert(rs.Count(), gc.Equals, 2)
37}37}
3838
39func (resourceSuite) TestRegisterNamedGetCount(c *gc.C) {
40 rs := common.NewResources()
41 defer rs.StopAll()
42 r1 := &fakeResource{}
43 err := rs.RegisterNamed("fake1", r1)
44 c.Assert(err, gc.IsNil)
45 c.Check(rs.Count(), gc.Equals, 1)
46 c.Check(rs.Get("fake1"), gc.Equals, r1)
47}
48
49func (resourceSuite) TestRegisterNamedRepeatedName(c *gc.C) {
50 rs := common.NewResources()
51 defer rs.StopAll()
52 r1 := &fakeResource{}
53 r2 := &fakeResource{}
54 err := rs.RegisterNamed("fake1", r1)
55 c.Assert(err, gc.IsNil)
56 c.Check(rs.Count(), gc.Equals, 1)
57 err = rs.RegisterNamed("fake1", r2)
58 c.Check(err, gc.ErrorMatches, `resource "fake1" already registered`)
59 c.Check(rs.Count(), gc.Equals, 1)
60 c.Check(rs.Get("fake1"), gc.Equals, r1)
61}
62
63func (resourceSuite) TestRegisterNamedIntegerName(c *gc.C) {
64 rs := common.NewResources()
65 defer rs.StopAll()
66 r1 := &fakeResource{}
67 err := rs.RegisterNamed("1", r1)
68 c.Check(err, gc.ErrorMatches, `RegisterNamed does not allow integer names: "1"`)
69 c.Check(rs.Count(), gc.Equals, 0)
70 c.Check(rs.Get("fake1"), gc.IsNil)
71}
72
73func (resourceSuite) TestRegisterNamedIntegerStart(c *gc.C) {
74 rs := common.NewResources()
75 defer rs.StopAll()
76 r1 := &fakeResource{}
77 err := rs.RegisterNamed("1fake", r1)
78 c.Assert(err, gc.IsNil)
79 c.Check(rs.Count(), gc.Equals, 1)
80 c.Check(rs.Get("1fake"), gc.Equals, r1)
81}
82
39func (resourceSuite) TestConcurrency(c *gc.C) {83func (resourceSuite) TestConcurrency(c *gc.C) {
40 // This test is designed to cause the race detector84 // This test is designed to cause the race detector
41 // to fail if the locking is not done correctly.85 // to fail if the locking is not done correctly.
@@ -54,6 +98,9 @@
54 rs.Register(&fakeResource{})98 rs.Register(&fakeResource{})
55 })99 })
56 start(func() {100 start(func() {
101 rs.RegisterNamed("named", &fakeResource{})
102 })
103 start(func() {
57 rs.Stop("1")104 rs.Stop("1")
58 })105 })
59 start(func() {106 start(func() {
@@ -65,6 +112,9 @@
65 start(func() {112 start(func() {
66 rs.Get("2")113 rs.Get("2")
67 })114 })
115 start(func() {
116 rs.Get("named")
117 })
68 wg.Wait()118 wg.Wait()
69}119}
70120
71121
=== modified file 'state/apiserver/pinger_test.go'
--- state/apiserver/pinger_test.go 2014-04-04 16:52:41 +0000
+++ state/apiserver/pinger_test.go 2014-05-20 08:25:48 +0000
@@ -86,6 +86,25 @@
86 c.Assert(err, gc.ErrorMatches, "connection is shut down")86 c.Assert(err, gc.ErrorMatches, "connection is shut down")
87}87}
8888
89func (s *stateSuite) TestAgentConnectionDelaysShutdownWithPing(c *gc.C) {
90 // We have to be careful, because Login can take 25ms, so we ping
91 // immediately after connecting.
92 s.PatchValue(apiserver.MaxClientPingInterval, 50*time.Millisecond)
93 st, _ := s.OpenAPIAsNewMachine(c)
94 err := st.Ping()
95 c.Assert(err, gc.IsNil)
96 // As long as we don't wait too long, the connection stays open
97 for i := 0; i < 10; i++ {
98 time.Sleep(10 * time.Millisecond)
99 err = st.Ping()
100 c.Assert(err, gc.IsNil)
101 }
102 // However, once we stop pinging for too long, the connection dies
103 time.Sleep(75 * time.Millisecond)
104 err = st.Ping()
105 c.Assert(err, gc.ErrorMatches, "connection is shut down")
106}
107
89type mongoPingerSuite struct {108type mongoPingerSuite struct {
90 testing.JujuConnSuite109 testing.JujuConnSuite
91}110}
92111
=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go 2014-05-14 16:07:11 +0000
+++ state/apiserver/root.go 2014-05-20 08:25:48 +0000
@@ -56,10 +56,9 @@
56// after it has logged in.56// after it has logged in.
57type srvRoot struct {57type srvRoot struct {
58 clientAPI58 clientAPI
59 srv *Server59 srv *Server
60 rpcConn *rpc.Conn60 rpcConn *rpc.Conn
61 resources *common.Resources61 resources *common.Resources
62 pingTimeout *pingTimeout
6362
64 entity taggedAuthenticator63 entity taggedAuthenticator
65}64}
@@ -82,9 +81,6 @@
82// cleaning up to ensure that all outstanding requests return.81// cleaning up to ensure that all outstanding requests return.
83func (r *srvRoot) Kill() {82func (r *srvRoot) Kill() {
84 r.resources.StopAll()83 r.resources.StopAll()
85 if r.pingTimeout != nil {
86 r.pingTimeout.stop()
87 }
88}84}
8985
90// requireAgent checks whether the current client is an agent and hence86// requireAgent checks whether the current client is an agent and hence
@@ -347,10 +343,11 @@
347// is not called frequently enough, the connection343// is not called frequently enough, the connection
348// will be dropped.344// will be dropped.
349func (r *srvRoot) Pinger(id string) (pinger, error) {345func (r *srvRoot) Pinger(id string) (pinger, error) {
350 if r.pingTimeout == nil {346 pingTimeout, ok := r.resources.Get("pingTimeout").(pinger)
347 if !ok {
351 return nullPinger{}, nil348 return nullPinger{}, nil
352 }349 }
353 return r.pingTimeout, nil350 return pingTimeout, nil
354}351}
355352
356type nullPinger struct{}353type nullPinger struct{}
@@ -438,8 +435,8 @@
438 }435 }
439}436}
440437
441// stop terminates the ping timeout.438// Stop terminates the ping timeout.
442func (pt *pingTimeout) stop() error {439func (pt *pingTimeout) Stop() error {
443 pt.tomb.Kill(nil)440 pt.tomb.Kill(nil)
444 return pt.tomb.Wait()441 return pt.tomb.Wait()
445}442}
446443
=== modified file 'state/apiserver/root_test.go'
--- state/apiserver/root_test.go 2014-01-22 19:28:08 +0000
+++ state/apiserver/root_test.go 2014-05-20 08:25:48 +0000
@@ -58,7 +58,6 @@
58 // Expect action to be executed about 50ms after last ping.58 // Expect action to be executed about 50ms after last ping.
59 broken := time.Now()59 broken := time.Now()
60 var closed time.Time60 var closed time.Time
61 time.Sleep(100 * time.Millisecond)
62 select {61 select {
63 case closed = <-closedc:62 case closed = <-closedc:
64 case <-time.After(testing.LongWait):63 case <-time.After(testing.LongWait):
@@ -67,3 +66,19 @@
67 closeDiff := closed.Sub(broken) / time.Millisecond66 closeDiff := closed.Sub(broken) / time.Millisecond
68 c.Assert(50 <= closeDiff && closeDiff <= 100, gc.Equals, true)67 c.Assert(50 <= closeDiff && closeDiff <= 100, gc.Equals, true)
69}68}
69
70func (r *rootSuite) TestPingTimeoutStopped(c *gc.C) {
71 closedc := make(chan time.Time, 1)
72 action := func() {
73 closedc <- time.Now()
74 }
75 timeout := apiserver.NewPingTimeout(action, 20*time.Millisecond)
76 timeout.Ping()
77 timeout.Stop()
78 // The action should never trigger
79 select {
80 case <-closedc:
81 c.Fatalf("action triggered after Stop()")
82 case <-time.After(testing.ShortWait):
83 }
84}

Subscribers

People subscribed via source and target branches

to status/vote changes: