Have the peergrouper publish the apiservers over the central hub. #6669

Merged
merged 4 commits into from Dec 9, 2016

Conversation

Projects
None yet
4 participants
Owner

howbazaar commented Dec 7, 2016

Also refactors the peergrouper to use clock.Clock interface rather than time directly.

Looks good, other than the tyop.

worker/peergrouper/worker.go
@@ -101,11 +111,15 @@ type pgWorker struct {
publisher publisherInterface
providerSupportsSpaces bool
+
+ // hub is the central hub of the apiserver, and is used to pusblish the
+// Hub defines the only method of the apiserver centralhub that
+// the peer grouper uses.
+type Hub interface {
+ Publish(topic pubsub.Topic, data interface{}) (<-chan struct{}, error)
@natefinch

natefinch Dec 8, 2016

Contributor

If you made Publish take a simple string instead of a pubsub.Topic, you could avoid importing pubsub here (and everywhere else that defines a similar interface).

The only benefit to the Topic type is that it's already a TopicMatcher.... but all that does is prevent you from needing two subscribes - one for a single topic (the common case) and one with a matcher (less common).

If you instead wrote publish & subscribe like this:

Publish(topic string, data interface{}) (<-chan struct{}, error)
Subscribe(topic string, handler func(string, interface{})) (unsubscribe func())
SubscribeMatches(match func(string)bool, handler func(string, interface{})) (unsubscribe func())

Then things that get dependency injected wouldn't need to import pubsub at all.

Also then you wouldn't need to import pubsub when you define a topic and its types, as in the file above (messages.go)

Owner

howbazaar commented Dec 9, 2016

$$merge$$

Contributor

jujubot commented Dec 9, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Dec 9, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9834

Owner

howbazaar commented Dec 9, 2016

$$merge$$

Contributor

jujubot commented Dec 9, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit b7374ef into juju:develop Dec 9, 2016

@howbazaar howbazaar deleted the howbazaar:centralhub-4-peergrouper branch Dec 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment