From c0a672c02a0485fab17f3ceaaf6c5c9bb7365eaa Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Fri, 1 Jul 2016 16:51:57 +0000 Subject: [PATCH] Review feedback --- app/controls_test.go | 2 +- app/pipes_internal_test.go | 2 +- probe/appclient/app_client.go | 15 ++++-------- probe/appclient/app_client_internal_test.go | 6 ++--- probe/appclient/multi_client.go | 26 ++++++++++----------- probe/appclient/multi_client_test.go | 4 ++-- prog/probe.go | 6 ++--- 7 files changed, 28 insertions(+), 33 deletions(-) diff --git a/app/controls_test.go b/app/controls_test.go index 34ee06fbf..2e1a7c520 100644 --- a/app/controls_test.go +++ b/app/controls_test.go @@ -43,7 +43,7 @@ func TestControl(t *testing.T) { Value: "foo", } }) - client, err := appclient.NewAppClient(probeConfig, ip+":"+port, ip+":"+port, controlHandler, false) + client, err := appclient.NewAppClient(probeConfig, ip+":"+port, ip+":"+port, controlHandler) if err != nil { t.Fatal(err) } diff --git a/app/pipes_internal_test.go b/app/pipes_internal_test.go index d01e3960e..308919755 100644 --- a/app/pipes_internal_test.go +++ b/app/pipes_internal_test.go @@ -83,7 +83,7 @@ func TestPipeClose(t *testing.T) { probeConfig := appclient.ProbeConfig{ ProbeID: "foo", } - client, err := appclient.NewAppClient(probeConfig, ip+":"+port, ip+":"+port, nil, false) + client, err := appclient.NewAppClient(probeConfig, ip+":"+port, ip+":"+port, nil) if err != nil { t.Fatal(err) } diff --git a/probe/appclient/app_client.go b/probe/appclient/app_client.go index a66a95dcd..bffaf0d5c 100644 --- a/probe/appclient/app_client.go +++ b/probe/appclient/app_client.go @@ -57,12 +57,11 @@ type appClient struct { readers chan io.Reader // For controls - noControls bool - control xfer.ControlHandler + control xfer.ControlHandler } // NewAppClient makes a new appClient. -func NewAppClient(pc ProbeConfig, hostname, target string, control xfer.ControlHandler, noControls bool) (AppClient, error) { +func NewAppClient(pc ProbeConfig, hostname, target string, control xfer.ControlHandler) (AppClient, error) { httpTransport, err := pc.getHTTPTransport(hostname) if err != nil { return nil, err @@ -83,10 +82,9 @@ func NewAppClient(pc ProbeConfig, hostname, target string, control xfer.ControlH wsDialer: websocket.Dialer{ TLSClientConfig: httpTransport.TLSClientConfig, }, - conns: map[string]xfer.Websocket{}, - readers: make(chan io.Reader, 2), - noControls: noControls, - control: control, + conns: map[string]xfer.Websocket{}, + readers: make(chan io.Reader, 2), + control: control, }, nil } @@ -233,9 +231,6 @@ func (c *appClient) controlConnection() (bool, error) { } func (c *appClient) ControlConnection() { - if c.noControls { - return - } go func() { log.Infof("Control connection to %s starting", c.target) defer log.Infof("Control connection to %s exiting", c.target) diff --git a/probe/appclient/app_client_internal_test.go b/probe/appclient/app_client_internal_test.go index 5d8cd149a..d8acc0ed7 100644 --- a/probe/appclient/app_client_internal_test.go +++ b/probe/appclient/app_client_internal_test.go @@ -109,7 +109,7 @@ func TestAppClientPublish(t *testing.T) { Insecure: false, } - p, err := NewAppClient(pc, u.Host, s.URL, nil, false) + p, err := NewAppClient(pc, u.Host, s.URL, nil) if err != nil { t.Fatal(err) } @@ -158,7 +158,7 @@ func TestAppClientDetails(t *testing.T) { ProbeID: "", Insecure: false, } - p, err := NewAppClient(pc, u.Host, s.URL, nil, false) + p, err := NewAppClient(pc, u.Host, s.URL, nil) if err != nil { t.Fatal(err) } @@ -203,7 +203,7 @@ func TestStop(t *testing.T) { Insecure: false, } - p, err := NewAppClient(pc, u.Host, s.URL, nil, false) + p, err := NewAppClient(pc, u.Host, s.URL, nil) if err != nil { t.Fatal(err) } diff --git a/probe/appclient/multi_client.go b/probe/appclient/multi_client.go index 061373059..91428507b 100644 --- a/probe/appclient/multi_client.go +++ b/probe/appclient/multi_client.go @@ -23,11 +23,12 @@ type ClientFactory func(string, string) (AppClient, error) type multiClient struct { clientFactory ClientFactory - mtx sync.Mutex - sema semaphore - clients map[string]AppClient // holds map from app id -> client - ids map[string]report.IDList // holds map from hostname -> app ids - quit chan struct{} + mtx sync.Mutex + sema semaphore + clients map[string]AppClient // holds map from app id -> client + ids map[string]report.IDList // holds map from hostname -> app ids + quit chan struct{} + noControls bool } type clientTuple struct { @@ -53,14 +54,15 @@ type MultiAppClient interface { } // NewMultiAppClient creates a new MultiAppClient. -func NewMultiAppClient(clientFactory ClientFactory) MultiAppClient { +func NewMultiAppClient(clientFactory ClientFactory, noControls bool) MultiAppClient { return &multiClient{ clientFactory: clientFactory, - sema: newSemaphore(maxConcurrentGET), - clients: map[string]AppClient{}, - ids: map[string]report.IDList{}, - quit: make(chan struct{}), + sema: newSemaphore(maxConcurrentGET), + clients: map[string]AppClient{}, + ids: map[string]report.IDList{}, + quit: make(chan struct{}), + noControls: noControls, } } @@ -100,9 +102,7 @@ func (c *multiClient) Set(hostname string, endpoints []string) { hostIDs := report.MakeIDList() for tuple := range clients { hostIDs = hostIDs.Add(tuple.ID) - - _, ok := c.clients[tuple.ID] - if !ok { + if _, ok := c.clients[tuple.ID]; !ok && !c.noControls { c.clients[tuple.ID] = tuple.AppClient tuple.AppClient.ControlConnection() } diff --git a/probe/appclient/multi_client_test.go b/probe/appclient/multi_client_test.go index 98126c1cd..5a24519b5 100644 --- a/probe/appclient/multi_client_test.go +++ b/probe/appclient/multi_client_test.go @@ -67,7 +67,7 @@ func TestMultiClient(t *testing.T) { } ) - mp := appclient.NewMultiAppClient(factory) + mp := appclient.NewMultiAppClient(factory, false) defer mp.Stop() // Add two hostnames with overlapping apps, check we don't add the same app twice @@ -89,7 +89,7 @@ func TestMultiClient(t *testing.T) { } func TestMultiClientPublish(t *testing.T) { - mp := appclient.NewMultiAppClient(factory) + mp := appclient.NewMultiAppClient(factory, false) defer mp.Stop() sum := func() int { return a1.publish + a2.publish + b2.publish + b3.publish } diff --git a/prog/probe.go b/prog/probe.go index 657b73a35..f461664cb 100644 --- a/prog/probe.go +++ b/prog/probe.go @@ -113,13 +113,13 @@ func probeMain(flags probeFlags) { ProbeID: probeID, Insecure: flags.insecure, } - clients := appclient.NewMultiAppClient(func(hostname, endpoint string) (appclient.AppClient, error) { + clientFactory := func(hostname, endpoint string) (appclient.AppClient, error) { return appclient.NewAppClient( probeConfig, hostname, endpoint, xfer.ControlHandlerFunc(controls.HandleControlRequest), - flags.noControls, ) - }) + } + clients := appclient.NewMultiAppClient(clientFactory, flags.noControls) defer clients.Stop() dnsLookupFn := net.LookupIP