Conversation
| } | ||
|
|
||
| // JSON representation is deliberately truncated to keep the message size small | ||
| type NodeUpdateEvent struct { |
There was a problem hiding this comment.
just moved this code from the events package to make the dependencies easier
mjh1
changed the title
leszko
left a comment
leszko
left a comment
There was a problem hiding this comment.
Other 2 comments. Other than that LGTM
| NodeMetrics: make(map[string]NodeMetrics), | ||
| metricTimeout: metricTimeout, | ||
| ingestStreamTimeout: ingestStreamTimeout, | ||
| MetricsDB: nodeStatsDB, |
There was a problem hiding this comment.
IIUC This will be a different database than our metrics DB. Then, could you rename it to something like NodeStatsDB?
| continue | ||
| } | ||
|
|
||
| c.UpdateNodes(event.NodeID, event.NodeMetrics) |
There was a problem hiding this comment.
Do we actually need to keep any internal state for nodes? If we make a query to DB with every playback redirect request, then couldn't we reason everything from there?
There was a problem hiding this comment.
Yep sorry I totally forgot to mention refactoring that as a plan for another PR, in two minds about whether I should do that in this PR but I think since it's been approved I think I'll just do it now but in a fresh PR.
3 participants
Switch to postgres to store node stats for catabalancer. Previously when we used serf to propagate this data throughout the cluster we ran into bottlenecks with messages queueing up, it didn't seem it was designed for that sort of use case.
Future PRs:
RefreshNodes()at the moment it can be called multiple times concurrently, possibly just change it to a periodic refresh.