Store,Compactor: Thanos sharding#1583
Conversation
|
Nice! It looks awesome generally. Will try to look on it ASAP. |
bwplotka
left a comment
There was a problem hiding this comment.
Nice, generally it looks like a good start! Some suggestions (: Thanks for doing this!
pkg/compact/compact.go
Outdated
| return nil, errors.Wrapf(err, "downloading meta.json for %s", id) | ||
| } | ||
|
|
||
| // Check for block labels by relabeling. |
There was a problem hiding this comment.
Nice. However I would move it out of downloadMeta as we do more than method suggests which might be confusing. Let's do it after meta is downloaded? Also it's nice to do it after downloadMeta method as we check if it's not too fresh first.
pkg/query/storeset.go
Outdated
| // Append the storeType to the end of list, because the store gateway will be exposed external labels, | ||
| // we allow the duplicate external labels with different components. | ||
| if store.storeType != nil { | ||
| tsdbLabelSetStrings = append(tsdbLabelSetStrings, store.storeType.String()) |
There was a problem hiding this comment.
Hm, this sounds like a hack, there is no way to check if this guy is a storeGW inside checking deduplicated stuff?
There was a problem hiding this comment.
Sorry, what do you mean?
There was a problem hiding this comment.
We are adding some artificial labels to the labelset. Why? This sounds wrong? To allow duplicated store gateway labels we just need to avoid detecting this if store.storeType is Gateway, right? NOT
pkg/store/bucket.go
Outdated
| level.Debug(s.logger).Log("msg", "dropping block(drop in relabeling)", "block", id) | ||
| return os.RemoveAll(dir) | ||
| } | ||
| b.labels = labels.FromMap(processedLabels.Map()) |
There was a problem hiding this comment.
This is important because actually we get the labels after relabelling, which we agreed not doing in the first iteration (:
There was a problem hiding this comment.
So should we expose the original labels from meta.json? Maybe the action of relabeling will be labeldrop or labelkeep.
There was a problem hiding this comment.
Yes!
It may yes. But what's the use case to drop only some labels?
In fact it would be more useful to add new labels! This is great but there are some limitiation for it (e.g relabelling cannot generate 2 lablesets) . For now in this PR I would say we care ONLY if it exists or not exists. Does it sounds fair? (:
pkg/store/bucket.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| if len(s.relabelConfig) != 0 { |
There was a problem hiding this comment.
I would vote to precompute this every sync instead of computing this on every Info which is frequent (at least every 5s)
… gateway Signed-off-by: jojohappy <sarahdj0917@gmail.com>
…mpty labels when no relabel configuration Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
d420725 to
a210289
Compare
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
|
It would really nice to add tests particulary for sharding compactor and store GW. Maybe in |
bwplotka
left a comment
There was a problem hiding this comment.
Awesome! It looks amazing. Hopefully last things to fix!
Thanks for doing it!
CHANGELOG.md
Outdated
| - [#1533](https://github.com/thanos-io/thanos/pull/1533) Thanos inspect now supports the timeout flag. | ||
| - [#1362](https://github.com/thanos-io/thanos/pull/1362) Optional `replicaLabels` param for `/query` and `/query_range` querier endpoints. When provided overwrite the `query.replica-label` cli flags. | ||
| - [#1583](https://github.com/thanos-io/thanos/pull/1583) Thanos sharding: | ||
| - Add relabel config (`--selector.relabel-config-file` and `selector.relabel-config`) into Thanos |
There was a problem hiding this comment.
Worth to note exact components (:
CHANGELOG.md
Outdated
| - [#1583](https://github.com/thanos-io/thanos/pull/1583) Thanos sharding: | ||
| - Add relabel config (`--selector.relabel-config-file` and `selector.relabel-config`) into Thanos | ||
| - Selecting blocks to serve depends on the result of block labels relabeling. | ||
| - For store gateway, expose advertise labels after relabeling. |
There was a problem hiding this comment.
| - For store gateway, expose advertise labels after relabeling. | |
| - For store gateway, advertise labels from "approved" blocks. |
| - [#1362](https://github.com/thanos-io/thanos/pull/1362) Optional `replicaLabels` param for `/query` and `/query_range` querier endpoints. When provided overwrite the `query.replica-label` cli flags. | ||
| - [#1583](https://github.com/thanos-io/thanos/pull/1583) Thanos sharding: | ||
| - Add relabel config (`--selector.relabel-config-file` and `selector.relabel-config`) into Thanos | ||
| - Selecting blocks to serve depends on the result of block labels relabeling. |
There was a problem hiding this comment.
This should be part of the last sentence.
cmd/thanos/flags.go
Outdated
| return extflag.RegisterPathOrContent( | ||
| cmd, | ||
| "selector.relabel-config", | ||
| "YAML file that contains seletor relabeling configuration. It follows native Prometheus relabel-config syntax. See format details: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config ", |
There was a problem hiding this comment.
| "YAML file that contains seletor relabeling configuration. It follows native Prometheus relabel-config syntax. See format details: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config ", | |
| "YAML file that contains relabeling configuration that allows selecting blocks. It follows native Prometheus relabel-config syntax. See format details: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config ", |
pkg/query/storeset.go
Outdated
| externalLabelOccurrencesInStores := map[string]int{} | ||
| for _, st := range healthyStores { | ||
| externalLabelOccurrencesInStores[externalLabelsFromStore(st)]++ | ||
| if st.storeType != nil && (st.storeType.ToProto() == storepb.StoreType_QUERY || |
There was a problem hiding this comment.
Why are we changing this? Let's maybe focus on line 258 as you did instead (:
pkg/query/storeset.go
Outdated
| if len(store.LabelSets()) > 0 && externalLabelOccurrencesInStores[externalLabels] != 1 { | ||
| if len(store.LabelSets()) > 0 && | ||
| store.storeType != nil && | ||
| (store.storeType.ToProto() == storepb.StoreType_QUERY || |
There was a problem hiding this comment.
Can we just exclude StoreType_STORE? It will be much safe. It's the only thing that we change here - store GW handing, right?
pkg/store/bucket.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| labelSets := make(map[uint64][]storepb.Label, len(s.blocks)) |
There was a problem hiding this comment.
Again as I mentioned before - we can build that every sync instead of every Info call. Can we do that? (:
There was a problem hiding this comment.
Info should be as fast as possible as it works as healthiness/heartbeat check.
There was a problem hiding this comment.
I will update today. Thanks!
|
It would be also awesome to have some tests for compactor filtering and store GW exposing info + filtering. Maybe in next PR (: |
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
295b12d to
de9a47f
Compare
|
Tests fails ): |
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
ecf1456 to
148828d
Compare
|
@bwplotka That's green! |
* Add selector.relabel-config flag && expose advertised label for store gateway Signed-off-by: jojohappy <sarahdj0917@gmail.com> Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
|
Hi @jojohappy @bwplotka |
Changes
Implement partial parts of proposal #1501 :
Note: pre-filter has been implemented before this PR.
TODO:
@bwplotka @brancz @povilasv @GiedriusS PTAL, thanks!