store: close bkt properly #5020
Conversation
Just a small fix: add bkt closing to the run group instead of closing it at the of the function that only sets things up. In practice, this didn't cause a bug because Close() is a noop. But as good Go programmers, let's Close() the bucket properly at the end of Thanos Store's lifetime. Signed-off-by: Giedrius Statkevičius <[email protected]>
cmd/thanos/store.go
Outdated
| runutil.CloseWithLogOnErr(logger, bkt, "bucket client") | ||
| } | ||
| }() | ||
| { |
There was a problem hiding this comment.
I'm trying to follow the code - it looks like we're also closing bkt in another run group below at 365. Do we need both?
Also, are other components using bucket clients adhering to this or are they using simply defer instead as well?
There was a problem hiding this comment.
You are correct, I have missed that one. We were talking about this particular code snippet that I've edited in our meetings with Prem & Akansha so I haven't even bothered to check other places. With this, we can simply remove this code block. I don't know about all clients but I think it's customary to call Close() at the end of using some resource.
cmd/thanos/store.go
Outdated
| done := make(chan struct{}) | ||
| g.Add(func() error { | ||
| <-done | ||
| return bkt.Close() |
There was a problem hiding this comment.
Should we retain CloseWithLogOnErr here so that we can log the potential close error?
This is already covered by: ``` defer runutil.CloseWithLogOnErr(logger, bkt, "bucket client") ``` In the same file. Signed-off-by: Giedrius Statkevičius <[email protected]>
Just a small fix: add bkt closing to the run group instead of closing it
at the of the function that only sets things up. In practice, this
didn't cause a bug because Close() is a noop. But as good Go
programmers, let's Close() the bucket properly at the end of Thanos
Store's lifetime.