Reduce mist client timeout for catabalancer stats sending #1400
Conversation
I saw occasional slowness from the mist GetState function. The mist client has a 1 minute timeout and we need a much shorter timeout here as the loop runs every 5 seconds
|
The diff is best viewed with whitespace hidden |
| sysusage, err := GetSystemUsage() | ||
| if err != nil { | ||
| log.LogNoRequestID("catabalancer failed to get sys usage", "err", err) | ||
| done := make(chan bool) |
There was a problem hiding this comment.
I think the more "golang way" of doing the same would be passing context with timeout into sendMetrics(). So you could have something like this:
ctx, cancel := context.WithTimeout(context.Background(), timeout)
sendMetrics(ctx, nodeName, latitude, longitude, mist, nodeStatsDB)And then inside sendMetrics() to have
GetSystemUsage(ctx)And so on. The good thing about this approach would be that in case of the timeout you cancel all system and network calls.
There was a problem hiding this comment.
Yeah.. I see what you mean, unfortunately I couldn't find any funcs within GetSystemUsage which take a context so it really is just the mist client we need to have the timeout for, and so you've made me realise the proper solution is really to reduce the timeout for the mist client
There was a problem hiding this comment.
@leszko I've pretty much re-done this PR, please could you take a look? thanks
mjh1
force-pushed
the
mh/catabalancer
branch
3 times, most recently
from
91129cb to
7c9b496
Compare
main.go
Outdated
| @@ -267,6 +267,7 @@ func main() { | |||
|
|
|||
| if catabalancerEnabled && nodeStatsDB != nil { | |||
| if cli.Tags["node"] == "media" { // don't announce load balancing availability for testing nodes | |||
| mist := clients.NewMistAPIClient(cli.MistUser, cli.MistPassword, cli.MistHost, cli.MistPort, catabalancer.StatsUpdateInterval-time.Second) | |||
There was a problem hiding this comment.
Could you add a comment with the reasoning why it's catabalancer.StatsUpdateInterval-time.Second? It's not clear to me.
mjh1
changed the title
I saw occasional slowness from the mist GetState function. The mist client has a 1 minute timeout and we need a much shorter timeout here as the loop runs every 5 seconds