Added hashmod support for store sharding. #2172
Conversation
pkg/block/fetcher.go
Outdated
| @@ -401,14 +401,25 @@ func NewLabelShardedMetaFilter(relabelConfig []*relabel.Config) *LabelShardedMet | |||
| return &LabelShardedMetaFilter{relabelConfig: relabelConfig} | |||
| } | |||
|
|
|||
| const blockIDLabel = "__block_id" | |||
|
|
|||
| // Filter filters out blocks that filters blocks that have no labels after relabelling. | |||
There was a problem hiding this comment.
This comment is quite confusing to me. Do the filtered blocks filter out other blocks with no labels? or is there an extra filters out blocks here?
There was a problem hiding this comment.
Yes I think there is a double filters out blocks.
pkg/block/fetcher.go
Outdated
| lbls[0] = labels.Label{Name: blockIDLabel, Value: id.String()} | ||
| lbls = lbls[:1] | ||
| if len(lbls) < len(m.Thanos.Labels) { | ||
| lbls = make([]labels.Label, 0, len(m.Thanos.Labels)) |
There was a problem hiding this comment.
this logic is a bit funny I think. If m.Thanos.Labels has length 2, then we throw away the block ID label we set earlier. But if it has length 1, then we keep the block ID label and allocate a new backing array when we append that one single label. Is that what we want to do?
There was a problem hiding this comment.
Yeah it looks like in this case we wouldn't end up having the block id label I think. I can see what you were trying to do but is this really that performance sensitive that we need to safe the allocations here?
There was a problem hiding this comment.
Good point. I wanted to save allocs, will fix
|
Addressed all, thanks for detailed review 👍 |
pkg/block/fetcher.go
Outdated
| if processedLabels := relabel.Process(labels.FromMap(m.Thanos.Labels), f.relabelConfig...); processedLabels != nil { | ||
| continue | ||
| lbls = lbls[:0] | ||
| if len(lbls) < len(m.Thanos.Labels)+1 { |
There was a problem hiding this comment.
you reset the length in the line above, I assume you meant to do cap(lbls) on the left side here?
pkg/block/fetcher.go
Outdated
| if processedLabels := relabel.Process(labels.FromMap(m.Thanos.Labels), f.relabelConfig...); processedLabels != nil { | ||
| continue | ||
| lbls = lbls[:0] | ||
| if len(lbls) < len(m.Thanos.Labels)+1 { |
There was a problem hiding this comment.
This check is now unnecessary. Here, lbls will ALWAYS be 0 length, and len(m.Thanos.Labels)+1 will ALWAYS be at least 1, so the make will always be called. I think we can skip the if and simply always call the lbls = make...
There was a problem hiding this comment.
or just remove all of this and just reset lbls at the beginning of the for body, then append will take care of growing the array when needed
There was a problem hiding this comment.
Wanted to reuse the arr. Fixed now
bwplotka
force-pushed
the
sharding
branch
2 times, most recently
from
93e0515
to
e4d81ef
Compare
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
|
At some point, we need benchmarks for those filters. Maybe we would need to make this more concurrently etc. TODO: #2174 |
|
I assume you’re mostly talking about fetching the block meta. Relabeling is cheap, fast and cacheable. |
|
No, I mean filtering itself. Especially the graph-based one for |
|
Makes sense. |
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
No description provided.