Move TCP UDP start up into server.Start()
#4903
Conversation
|
Can one of the admins verify this patch? |
vjsamuel
changed the title
server.Start()server.Start()
vjsamuel
force-pushed
the
move_tcp_udp_startup
branch
from
8636fe8
to
a958b7f
Compare
metricbeat/helper/server/tcp/tcp.go
Outdated
| return &TcpServer{ | ||
| listener: listener, | ||
| tcpAddr: addr, | ||
| receiveBufferSize: config.ReceiveBufferSize, | ||
| done: make(chan struct{}), | ||
| eventQueue: make(chan server.Event), | ||
| }, nil | ||
| } | ||
|
|
||
| func (g *TcpServer) Start() { |
There was a problem hiding this comment.
I think this should return the error to the caller if Start() fails. Then the caller can at least know that it's not really listening and do something (like log and stop).
There was a problem hiding this comment.
@andrewkroh, addressed the review comment.
vjsamuel
force-pushed
the
move_tcp_udp_startup
branch
from
a958b7f
to
2409b2b
Compare
|
@vjsamuel please check the test output. It seems there is a panic in netpoll after this change. |
metricbeat/helper/server/tcp/tcp.go
Outdated
| go g.WatchMetrics() | ||
| return nil | ||
|
|
||
| } | ||
|
|
||
| func (g *TcpServer) WatchMetrics() { |
There was a problem hiding this comment.
Should this be unexported? I assumer users must call NewX, Start, GetEvents, and lastly Stop.
| m.server.Start() | ||
| err := m.server.Start() | ||
| if err != nil { | ||
| logp.Err("%v", err) |
There was a problem hiding this comment.
Do you want to add some context to the error message like "graphite server failed to start: %v"?
metricbeat/helper/server/udp/udp.go
Outdated
| func (g *UdpServer) Start() error { | ||
| listener, err := net.ListenUDP("udp", g.udpaddr) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to start UDP server with error: %v", err) |
There was a problem hiding this comment.
I would prefer errors.Wrap(err, "failed to start UDP server") over fmt.Errorf (with errors being github.com/pkg/errors). It retains the original error and also adds stack trace information that can be logged with %+v.
| @@ -61,7 +62,11 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | |||
| // Run method provides the Graphite server with a reporter with which events can be reported. | |||
| func (m *MetricSet) Run(reporter mb.PushReporter) { | |||
| // Start event watcher | |||
| m.server.Start() | |||
| err := m.server.Start() | |||
| if err != nil { | |||
There was a problem hiding this comment.
You could tighten this up if err := m.server.Start(); err != nil {...
| m.server.Start() | ||
| err := m.server.Start() | ||
| if err != nil { | ||
| logp.Err("%v", err) |
There was a problem hiding this comment.
You might find it useful to report the error to the output (e.g. reporter.Error(err)). Then you will the info that one of the graphite listeners failed sent to your output.
err = errors.Wrap(err, "failed to start graphite server")
logp.Err("%v, err)
reporter.Error(err)
return
2409b2b
to
652d6d0
Compare
metricbeat/helper/server/udp/udp.go
Outdated
| func (g *UdpServer) Start() error { | ||
| listener, err := net.ListenUDP("udp", g.udpaddr) | ||
| if err != nil { | ||
| return errors.Wrap(err, "Failed to start UDP server") |
There was a problem hiding this comment.
Error strings should not be capitalized. https://github.com/golang/go/wiki/CodeReviewComments#error-strings
vjsamuel
force-pushed
the
move_tcp_udp_startup
branch
from
652d6d0
to
b0b5197
Compare
vjsamuel
force-pushed
the
move_tcp_udp_startup
branch
2 times, most recently
from
588d528
to
2fff7b9
Compare
vjsamuel
force-pushed
the
move_tcp_udp_startup
branch
from
2fff7b9
to
8bad167
Compare
Making this change since
NewTcpServerandNewUdpServeropen the port which is not the ideal scenario.