Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

querier: Dedup series is now replica label agnostic and simpler. Fixed panic seen when using larger number of replicas and small series. #2728

Merged
merged 1 commit into from Jun 10, 2020
Merged

querier: Dedup series is now replica label agnostic and simpler. Fixed panic seen when using larger number of replicas and small series. #2728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
- [#2637](https://github.com/thanos-io/thanos/pull/2637) Compact: detect retryable errors that are inside of a wrapped `tsdb.MultiError`
- [#2648](https://github.com/thanos-io/thanos/pull/2648) Store: allow index cache and caching bucket to be configured at the same time.
- [#2705](https://github.com/thanos-io/thanos/pull/2705) minio-go: Added support for `af-south-1` and `eu-south-1` regions.
- [#2728](https://github.com/thanos-io/thanos/pull/2728) Query: Fixed panics when using larger number of replica labels with short series label sets.

### Changed

Expand Down
8 changes: 6 additions & 2 deletions pkg/query/iter.go
View file
Open in desktop
Expand Up @@ -381,8 +381,12 @@ func (s *dedupSeriesSet) peekLset() labels.Labels {
}
// Check how many replica labels are present so that these are removed.
var totalToRemove int
for index := 0; index < len(s.replicaLabels); index++ {
if _, ok := s.replicaLabels[lset[len(lset)-index-1].Name]; ok {
for i := 0; i < len(s.replicaLabels); i++ {
if len(lset)-i == 0 {
break
}

if _, ok := s.replicaLabels[lset[len(lset)-i-1].Name]; ok {
totalToRemove++
}
}
Expand Down
9 changes: 3 additions & 6 deletions pkg/query/querier.go
View file
Open in desktop
Expand Up @@ -214,20 +214,17 @@ func (q *querier) Select(_ bool, hints *storage.SelectHints, ms ...*labels.Match
}, warns, nil
}

// TODO(fabxc): this could potentially pushed further down into the store API
// to make true streaming possible.
// TODO(fabxc): this could potentially pushed further down into the store API to make true streaming possible.
sortDedupLabels(resp.seriesSet, q.replicaLabels)

set := &promSeriesSet{
mint: q.mint,
maxt: q.maxt,
set: newStoreSeriesSet(resp.seriesSet),
aggrs: aggrs,
}

// The merged series set assembles all potentially-overlapping time ranges
// of the same series into a single one. The series are ordered so that equal series
// from different replicas are sequential. We can now deduplicate those.
// The merged series set assembles all potentially-overlapping time ranges of the same series into a single one.
// TODO(bwplotka): We could potentially dedup on chunk level, use chunk iterator for that when available.
return newDedupSeriesSet(set, q.replicaLabels, len(aggrs) == 1 && aggrs[0] == storepb.Aggr_COUNTER), warns, nil
}

Expand Down
157 changes: 50 additions & 107 deletions pkg/query/querier_test.go
View file
Open in desktop
Expand Up @@ -800,123 +800,36 @@ func TestSortReplicaLabel(t *testing.T) {
// 0 Single deduplication label.
{
input: []storepb.Series{
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-1"},
{Name: "c", Value: "3"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-1"},
{Name: "c", Value: "3"},
{Name: "d", Value: "4"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-1"},
{Name: "c", Value: "4"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-2"},
{Name: "c", Value: "3"},
}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "3"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "4"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "c", Value: "3"}}},
},
exp: []storepb.Series{
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "b", Value: "replica-1"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "b", Value: "replica-2"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "d", Value: "4"},
{Name: "b", Value: "replica-1"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "4"},
{Name: "b", Value: "replica-1"},
}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-1"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}, {Name: "b", Value: "replica-1"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "4"}, {Name: "b", Value: "replica-1"}}},
},
dedupLabels: map[string]struct{}{"b": {}},
},
// 1 Multi deduplication labels.
{
input: []storepb.Series{
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-1"},
{Name: "b1", Value: "replica-1"},
{Name: "c", Value: "3"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-1"},
{Name: "b1", Value: "replica-1"},
{Name: "c", Value: "3"},
{Name: "d", Value: "4"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-1"},
{Name: "b1", Value: "replica-1"},
{Name: "c", Value: "4"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-2"},
{Name: "b1", Value: "replica-2"},
{Name: "c", Value: "3"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "b", Value: "replica-2"},
{Name: "c", Value: "3"},
}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "3"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "4"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "b1", Value: "replica-2"}, {Name: "c", Value: "3"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "c", Value: "3"}}},
},
exp: []storepb.Series{
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "b", Value: "replica-1"},
{Name: "b1", Value: "replica-1"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "b", Value: "replica-2"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "b", Value: "replica-2"},
{Name: "b1", Value: "replica-2"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "3"},
{Name: "d", Value: "4"},
{Name: "b", Value: "replica-1"},
{Name: "b1", Value: "replica-1"},
}},
{Labels: []storepb.Label{
{Name: "a", Value: "1"},
{Name: "c", Value: "4"},
{Name: "b", Value: "replica-1"},
{Name: "b1", Value: "replica-1"},
}},
},
dedupLabels: map[string]struct{}{
"b": {},
"b1": {},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}, {Name: "b1", Value: "replica-2"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "4"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
},
dedupLabels: map[string]struct{}{"b": {}, "b1": {}},
},
}
for _, test := range tests {
Expand Down Expand Up @@ -1007,6 +920,36 @@ func TestDedupSeriesSet(t *testing.T) {
"replica": {},
},
},
{
// Regression tests against: https://github.com/thanos-io/thanos/issues/2645.
// We were panicking on requests with more replica labels than overall labels in any series.
input: []series{
{
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-1"}},
samples: []sample{{10000, 1}, {20000, 2}},
}, {
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-2"}},
samples: []sample{{60000, 3}, {70000, 4}},
}, {
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-3"}},
samples: []sample{{100000, 10}, {150000, 20}},
}, {
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}},
samples: []sample{{10000, 1}, {20000, 2}},
},
},
exp: []series{
{
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}},
samples: []sample{{10000, 1}, {20000, 2}, {60000, 3}, {70000, 4}, {100000, 10}, {150000, 20}},
},
{
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}},
samples: []sample{{10000, 1}, {20000, 2}},
},
},
dedupLabels: map[string]struct{}{"replica": {}, "replica2": {}, "replica3": {}, "replica4": {}, "replica5": {}, "replica6": {}, "replica7": {}},
},
{
// Multi dedup label.
input: []series{
Expand Down Expand Up @@ -1131,7 +1074,7 @@ func TestDedupSeriesSet(t *testing.T) {
},
},
{
// Same thing but not for counter should not adjust antything.
// Same thing but not for counter should not adjust anything.
isCounter: false,
input: []series{
{
Expand Down