worker/peergrouper: publish API addresses

worker/peergrouper: publish API addresses

We only set the API addresses in the state for now.
Later, we will change the publish method to
publish in the environment too.

https://code.launchpad.net/~rogpeppe/juju-core/521-peergrouper-publish/+merge/211785

(do not edit description out of merge proposal)

[rog, 2014-03-19 17:27:20]

Please take a look.

[mfoord, 2014-03-19 17:36:41]

https://codereview.appspot.com/77600048/diff/1/worker/peergrouper/worker.go
File worker/peergrouper/worker.go (right):

https://codereview.appspot.com/77600048/diff/1/worker/peergrouper/worker.go#n...
worker/peergrouper/worker.go:45: // publish publishes information about the
given state servers
Should be"publishAPIServers publishes..."

[dimitern, 2014-03-20 08:55:05]

Looks great so far, but I have a few suggestions.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/mock_te...
File worker/peergrouper/mock_test.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/mock_te...
worker/peergrouper/mock_test.go:300: panic(err)
Should we really panic here?

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/publish.go
File worker/peergrouper/publish.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/publish...
worker/peergrouper/publish.go:1: package peergrouper
copyright header

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/publish...
worker/peergrouper/publish.go:19: // TODO publish API addresses (instance ids?)
in environment storage.
// TODO(rog) ...

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker.go
File worker/peergrouper/worker.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:175: logger.Errorf("cannot publish state server
addresses: %v", err)
s/state/API/ ?

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:186: logger.Infof("polling after %v", pollInterval)
Shouldn't this be a logger.Debugf?

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:192: logger.Infof("retrying after %v",
retryInterval)
ditto

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:501: 
d

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
File worker/peergrouper/worker_test.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:40: <-watcher.Changes()
Why not use statetesting package for asserts on the watcher?

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:284: func (f publisherFunc)
publishAPIServers(apiServers [][]instance.HostPort) error {
That's amazing :)

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:323: testbase.PatchValue(&pollInterval,
coretesting.LongWait+time.Second)
Why LongWait+1 second?

[rog, 2014-03-20 12:12:59]

Please take a look.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/mock_te...
File worker/peergrouper/mock_test.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/mock_te...
worker/peergrouper/mock_test.go:300: panic(err)
On 2014/03/20 08:55:05, dimitern wrote:
> Should we really panic here?

it can only be called from tests, and if tests shouldn't
be calling it with an invalid address - if they do,
i think a panic is ok. we could pass in gc.C and
assert, but it feels unnecessary to me.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/publish.go
File worker/peergrouper/publish.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/publish...
worker/peergrouper/publish.go:1: package peergrouper
On 2014/03/20 08:55:05, dimitern wrote:
> copyright header

Done.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/publish...
worker/peergrouper/publish.go:19: // TODO publish API addresses (instance ids?)
in environment storage.
On 2014/03/20 08:55:05, dimitern wrote:
> // TODO(rog) ...

Done.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker.go
File worker/peergrouper/worker.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:175: logger.Errorf("cannot publish state server
addresses: %v", err)
On 2014/03/20 08:55:05, dimitern wrote:
> s/state/API/ ?

Done.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:186: logger.Infof("polling after %v", pollInterval)
On 2014/03/20 08:55:05, dimitern wrote:
> Shouldn't this be a logger.Debugf?

removed

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:192: logger.Infof("retrying after %v",
retryInterval)
On 2014/03/20 08:55:05, dimitern wrote:
> ditto

removed

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker....
worker/peergrouper/worker.go:501: 
On 2014/03/20 08:55:05, dimitern wrote:
> d

Done.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
File worker/peergrouper/worker_test.go (right):

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:40: <-watcher.Changes()
On 2014/03/20 08:55:05, dimitern wrote:
> Why not use statetesting package for asserts on the watcher?

Done.

https://codereview.appspot.com/77600048/diff/20001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:323: testbase.PatchValue(&pollInterval,
coretesting.LongWait+time.Second)
On 2014/03/20 08:55:05, dimitern wrote:
> Why LongWait+1 second?

because if our logic is wrong and we use the poll interval rather than a retry
interval (something that actually happened in an earlier version of the code)
then we want to be sure we time out before the poll interval kicks in, and the
timeout is LongWait.

[dimitern, 2014-03-20 12:36:41]

LGTM, thanks!