From 68bca981de2f6bfda5b767af22744344f882aa73 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Wed, 5 Nov 2014 22:45:01 -0800 Subject: [PATCH] discovery: simplify interface There's no real need to expose a Discoverer interface/struct when the only use of the interface (and indeed the module) is to invoke a single function. This isn't Java, after all. So instead, simplify to Discovery exposing just two functions: JoinCluster (i.e. what was formerly called "discovery"), and GetCluster (hitherto "ProxyDiscovery") --- discovery/discovery.go | 68 ++++++++++++++++--------------------- discovery/discovery_test.go | 2 +- etcdmain/etcd.go | 6 +--- etcdserver/server.go | 6 +--- 4 files changed, 32 insertions(+), 50 deletions(-) diff --git a/discovery/discovery.go b/discovery/discovery.go index dfb500480..30fdd9bb3 100644 --- a/discovery/discovery.go +++ b/discovery/discovery.go @@ -52,16 +52,29 @@ const ( nRetries = uint(3) ) -type Discoverer interface { - // Discover connects to a discovery service and retrieves a string - // describing an etcd cluster, and any error encountered. - Discover() (string, error) +// JoinCluster will connect to the discovery service at the given url, and +// register the server represented by the given id and config to the cluster +func JoinCluster(durl string, id types.ID, config string) (string, error) { + d, err := newDiscovery(durl, id) + if err != nil { + return "", err + } + return d.joinCluster(config) +} + +// GetCluster will connect to the discovery service at the given url and +// retrieve a string describing the cluster +func GetCluster(durl string) (string, error) { + d, err := newDiscovery(durl, 0) + if err != nil { + return "", err + } + return d.getCluster() } type discovery struct { cluster string id types.ID - config string c client.KeysAPI retries uint url *url.URL @@ -69,27 +82,6 @@ type discovery struct { clock clockwork.Clock } -type proxyDiscovery struct { - *discovery -} - -// New returns a Discoverer which will connect to the discovery service at -// the given url, and register the server represented by the given id and -// config to the cluster during the discovery process -func New(durl string, id types.ID, config string) (Discoverer, error) { - return newDiscovery(durl, id, config) -} - -// ProxyNew returns a Discoverer which will connect to the discovery service -// at the given url and retrieve a string describing the cluster -func ProxyNew(durl string) (Discoverer, error) { - d, err := newDiscovery(durl, 0, "") - if err != nil { - return nil, err - } - return &proxyDiscovery{d}, nil -} - // proxyFuncFromEnv builds a proxy function if the appropriate environment // variable is set. It performs basic sanitization of the environment variable // and returns any error encountered. @@ -119,7 +111,7 @@ func proxyFuncFromEnv() (func(*http.Request) (*url.URL, error), error) { return http.ProxyURL(proxyURL), nil } -func newDiscovery(durl string, id types.ID, config string) (*discovery, error) { +func newDiscovery(durl string, id types.ID) (*discovery, error) { u, err := url.Parse(durl) if err != nil { return nil, err @@ -137,23 +129,21 @@ func newDiscovery(durl string, id types.ID, config string) (*discovery, error) { dc := client.NewDiscoveryKeysAPI(c) return &discovery{ cluster: token, - id: id, - config: config, c: dc, + id: id, url: u, clock: clockwork.NewRealClock(), }, nil } -func (d *discovery) Discover() (string, error) { - // fast path: if the cluster is full, returns the error - // do not need to register itself to the cluster in this - // case. +func (d *discovery) joinCluster(config string) (string, error) { + // fast path: if the cluster is full, return the error + // do not need to register to the cluster in this case. if _, _, err := d.checkCluster(); err != nil { return "", err } - if err := d.createSelf(); err != nil { + if err := d.createSelf(config); err != nil { // Fails, even on a timeout, if createSelf times out. // TODO(barakmich): Retrying the same node might want to succeed here // (ie, createSelf should be idempotent for discovery). @@ -173,8 +163,8 @@ func (d *discovery) Discover() (string, error) { return nodesToCluster(all), nil } -func (pd *proxyDiscovery) Discover() (string, error) { - nodes, size, err := pd.checkCluster() +func (d *discovery) getCluster() (string, error) { + nodes, size, err := d.checkCluster() if err != nil { if err == ErrFullCluster { return nodesToCluster(nodes), nil @@ -182,16 +172,16 @@ func (pd *proxyDiscovery) Discover() (string, error) { return "", err } - all, err := pd.waitNodes(nodes, size) + all, err := d.waitNodes(nodes, size) if err != nil { return "", err } return nodesToCluster(all), nil } -func (d *discovery) createSelf() error { +func (d *discovery) createSelf(contents string) error { ctx, cancel := context.WithTimeout(context.Background(), client.DefaultRequestTimeout) - resp, err := d.c.Create(ctx, d.selfKey(), d.config, -1) + resp, err := d.c.Create(ctx, d.selfKey(), contents, -1) cancel() if err != nil { return err diff --git a/discovery/discovery_test.go b/discovery/discovery_test.go index 8501b645e..d47dc736f 100644 --- a/discovery/discovery_test.go +++ b/discovery/discovery_test.go @@ -322,7 +322,7 @@ func TestCreateSelf(t *testing.T) { for i, tt := range tests { d := discovery{cluster: "1000", c: tt.c} - if err := d.createSelf(); err != tt.werr { + if err := d.createSelf(""); err != tt.werr { t.Errorf("#%d: err = %v, want %v", i, err, nil) } } diff --git a/etcdmain/etcd.go b/etcdmain/etcd.go index 8899333c8..45c14b45b 100644 --- a/etcdmain/etcd.go +++ b/etcdmain/etcd.go @@ -297,11 +297,7 @@ func startProxy() error { } if *durl != "" { - d, err := discovery.ProxyNew(*durl) - if err != nil { - return fmt.Errorf("cannot init discovery %v", err) - } - s, err := d.Discover() + s, err := discovery.GetCluster(*durl) if err != nil { return err } diff --git a/etcdserver/server.go b/etcdserver/server.go index 0dbe65a3a..62a3c0e00 100644 --- a/etcdserver/server.go +++ b/etcdserver/server.go @@ -203,11 +203,7 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) { } m := cfg.Cluster.MemberByName(cfg.Name) if cfg.ShouldDiscover() { - d, err := discovery.New(cfg.DiscoveryURL, m.ID, cfg.Cluster.String()) - if err != nil { - return nil, fmt.Errorf("cannot init discovery %v", err) - } - s, err := d.Discover() + s, err := discovery.JoinCluster(cfg.DiscoveryURL, m.ID, cfg.Cluster.String()) if err != nil { return nil, err }