Code review comment for lp:~jameinel/juju-core/api-named-resources

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

« Back to merge proposal