Upgrade Thanos to latest master #3363
Conversation
| @@ -55,15 +56,15 @@ func (bqss *blockQuerierSeriesSet) Next() bool { | |||
| return false | |||
| } | |||
|
|
|||
| currLabels := bqss.series[bqss.next].Labels | |||
| currLabels := labelpb.ZLabelsToPromLabels(bqss.series[bqss.next].Labels) | |||
There was a problem hiding this comment.
I've a doubt. In this Thanos upgrade, bqss.series[bqss.next].Labels is a labelpb.ZLabel, which means the whole protobuf message []byte will be retained until the query execution completes. Don't we risk to end up utilising more memory than before? In queriers I'm more worried about memory utilisation than memory allocations rate.
Thoughts @pstibrany @bwplotka ?
There was a problem hiding this comment.
If I understand this correctly, problem you're suggesting should not be an issue in querier. Previously querier would receive the message and then extract individual strings (making a copy), and (possibly) discard the message. Now we skip the extraction part at the cost of keeping the message in memory longer. As long as we don't retain these labels somewhere, I think this should decrease memory usage. But it's risky, and we better keep an eye on it.
There was a problem hiding this comment.
(I think *storepb.Series already keep reference to entire original message via c.Raw.Data)
Turns out they make a copy. Perhaps this could be improved as well.
There was a problem hiding this comment.
I think we should better investigate / understand how this behave. If a query execution is slow, we may potentially increase the memory utilisation with this PR's change.
I'm going to merge this PR to unblock other work based on this, but let's keep an eye on it.
There was a problem hiding this comment.
As I mentioned offline. Thanks to ZLabels, it's really up to you. If you don't want to hold memory, just use labelpb.DeepCopy function to do copy manually (: But you are right if you hold labels for longer time, it's very likely you want to copy those.
There was a problem hiding this comment.
We are still experimenting, so you are right. Might be risky
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
upgrade-thanos
branch
from
dbe9c7c
to
9cc84b1
Compare
…rgid-ctx * 'master' of github.com:cortexproject/cortex: Enforce integration tests default flags config to never be overwritten (cortexproject#3370) Avoid deletion of blocks which are not shipped (cortexproject#3346) Upgrade Thanos to latest master (cortexproject#3363) Migrate CircleCI workflows to GitHub Actions (2/3) (cortexproject#3341) Remove comments that doesn't seem right (cortexproject#3361) add ingester interface (cortexproject#3352) Fail fast an ingester if unable to load existing TSDBs (cortexproject#3354) Fixed Gossip memberlist members joining when addresses are configured using DNS-based service discovery (cortexproject#3360) Export distributor method to get ingester replication set (cortexproject#3356) Correct link for Block Storage reference (cortexproject#3234) Added section on Cleaner. (cortexproject#3327) Update prometheus vendor to master (cortexproject#3345) adding GHA CI env variable check (cortexproject#3351) Add ingesters shuffle sharding support on the read path (cortexproject#3252)
What this PR does:
This builds upon #3271 and updates to the latest Thanos master to also get thanos-io/thanos#3321 in.
About the Thanos upgrade: I checked the whole
vendor/diff and, assuming no bug is invendor/github.com/thanos-io/thanos/pkg/store/labelpb/label.go, the rest of the changes should be fine.Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]