Clean up stale ingester connections #237
Conversation
| @@ -48,6 +48,8 @@ func main() { | |||
| if err != nil { | |||
| log.Fatalf("Error initializing distributor: %v", err) | |||
| } | |||
| go dist.Run() | |||
There was a problem hiding this comment.
I was discussing this with @jml yesterday: in general, I prefer if goroutines are launched inside the constructor, mainly because these components are used in multiple places and its a pain to have to repeat the code everywhere (for instance, you didn't do go run dist.Run() in the ruler or the querieir`). WDYT?
There was a problem hiding this comment.
Ah right. Totally missed that these new jobs now also use the distributor internally. Of course. Will fix.
In the past (in Prometheus) we've usually settled on leaving concurrency up to the caller, also in case of these kinds of constructors. The ruler does it like that too, but e.g. the ingester doesn't. I'm cool either way. I'll change the go to be in the constructor for now?
There was a problem hiding this comment.
Yeah stick it in the constructor please - and the rule should be changed too (https://github.com/weaveworks/cortex/blob/master/cmd/ruler/main.go#L44) - will file an issue for it.
|
Made the changes. |
|
LGTM! It won't auto deploy, I'll do it in a batch tomorrow with my other PRs. |
This only closes connections to ingesters which are unhealthy or gone, not for leaving ones, so this should be ok.
Fixes #217