Simplify compactor and respect resolutions #195
Conversation
|
Note that this does not yet create downsamplings on the fly during compaction. But running sequences of |
|
Rebased, PTAL |
| return errors.Wrap(err, "build compaction groups") | ||
| } | ||
| for _, g := range groups { | ||
| os.RemoveAll(dataDir) |
There was a problem hiding this comment.
why is that here? (: We don't need any cache anymore?
There was a problem hiding this comment.
... ahh are we caching only meta json in memory and download them + blocks on compaction only.
That makes sense, cool 👍
| // groupKey returns a unique identifier for the group the block belongs to. It considers | ||
| // the downsampling resolution and the block's labels. | ||
| func groupKey(meta *block.Meta) string { | ||
| return fmt.Sprintf("%d@%s", meta.Thanos.Downsample.Resolution, labels.FromMap(meta.Thanos.Labels)) |
There was a problem hiding this comment.
mhm.. you are not hashing it. Are we sure order will not be different every call to labels.FromMap ? (: Maybe hash is more safe?
There was a problem hiding this comment.
ok, checked that method. Output is sorted... OK 👍
There was a problem hiding this comment.
Yea, that's the general contract for all constructors from the labels package – and really for anywhere we deal with labels.
| } | ||
| return res | ||
| sort.Slice(res, func(i, j int) bool { | ||
| return res[i].Key() < res[j].Key() |
There was a problem hiding this comment.
Do we really want this order?
1m@block1
1m@block2
1m@block3
5m@block1
5m@block2
...
Maybe we should put labels in the front of groupKey? Then we group by labels first.
I don't know which one is better ;p Just checking if there are any rationales behind that order (:
There was a problem hiding this comment.
Mh, no strict rationale I guess. But it likely makes sense to treat lower resolution blocks with higher priority since the most recent ones are queried the most.
pkg/compact/compact.go
Outdated
| return g, nil | ||
| } | ||
|
|
||
| // Key returns an identifier for the group. | ||
| func (cg *Group) Key() string { | ||
| return fmt.Sprintf("%s@%d", cg.labels, cg.resolution) |
There was a problem hiding this comment.
hm, cannot we use the same function/form as in groupKey function?
There was a problem hiding this comment.
Switched form but left it as 2 functions. Just wasn't practical to consolidate.
fabxc
force-pushed
the
compactdownsample
branch
2 times, most recently
from
8357d16
to
4af3535
Compare
This extends the compactor to consider downsample resolutions to the compaction grouping. With this it is safe to run the compactor against a bucket with data of different resolutions.
Furthermore #183 is likely to be cased by semantical races in the compactor. The PR drastically simplifies the logic and gets rid of any disk caching of meta.json files and just rebuilds groupings on demand. It is very unlikely that this has any noticable performance impact.