Use already open blocks while compacting. #441
Conversation
This roughly halves the RAM requirements of compaction. Signed-off-by: Brian Brazil <brian.brazil@robustperception.io>
|
would you mind opening the same PR on Prometheus so we can run a bench test to see it in action? |
|
Nice @brian-brazil Just curious how you measured the RAM consumption? Profiling, flamegraph? Can see you added timings as well. Also just wondering in what case Prometheus had those blocks already opened? by what operation? |
|
Heap profiling. They'll already be loaded by Prometheus as they're normal blocks. |
|
|
||
| // Use already open blocks if we can, to avoid | ||
| // having the index data in memory twice. | ||
| for _, o := range open { |
There was a problem hiding this comment.
Can we simplify this by making a map of the open blocks?
There was a problem hiding this comment.
My thinking was there's usually only 10ish open blocks, so no need to optimise.
| Compact(dest string, dirs ...string) (ulid.ULID, error) | ||
| // Can optionally pass a list of already open blocks, | ||
| // to avoid having to reopen them. | ||
| Compact(dest string, dirs []string, open []*Block) (ulid.ULID, error) |
There was a problem hiding this comment.
Since this is changing public interface. Could you tag a minor version? We don't have any tags yet so maybe a good time to start now.
There was a problem hiding this comment.
Yeah , or maybe even 1.1.0 , since it is pretty stable now and I think go modules don't pull anything below 1.0. It would be nice if we could use these tags as well when we vendor it in Prometheus.
There was a problem hiding this comment.
I think go modules don't pull anything below 1.0
Go modules do work with v0.x.y tags. Cutting a v1 is a pretty heavy commitment and I tend to think that a v0.something is appropriate here.
There was a problem hiding this comment.
I don't mind either way as long as we put something in place in terms of API versioning it is a step in the right direction.
There was a problem hiding this comment.
Okay, so I should merge and then add a v0.1.0 tag?
There was a problem hiding this comment.
yep lets try something as simple as that, unless @simonpasquier has any other recommendations.
I will keep reading how are we suppose to do this properly, but for now it is ok to start with just this.
There was a problem hiding this comment.
Okay, so I should merge and then add a v0.1.0 tag?
Hm.. you don't want to tag 0.1.0 before changing API?
There was a problem hiding this comment.
We've changed the API many times before, we have to start somewhere.
This roughly halves the RAM requirements of compaction.
There's not much point in optimising compaction memory itself further, as loading in the new blocks would OOM at reload time. Reducing the memory used by the indexes would be the main hope.
This also makes compaction a little bit faster.