Skip to content

Commit 5da2efd

Browse files
bwplotkabwplotka
committed
querier: Dedup series is now replica label agnostic and simpler. Fixed panic seen when using larger number of replicas and small series.
Fixes #2645 Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
1 parent 6f2c3b1 commit 5da2efd

File tree

5 files changed

+62
-595
lines changed

5 files changed

+62
-595
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
1616
- [#2637](https://github.com/thanos-io/thanos/pull/2637) Compact: detect retryable errors that are inside of a wrapped `tsdb.MultiError`
1717
- [#2648](https://github.com/thanos-io/thanos/pull/2648) Store: allow index cache and caching bucket to be configured at the same time.
1818
- [#2705](https://github.com/thanos-io/thanos/pull/2705) minio-go: Added support for `af-south-1` and `eu-south-1` regions.
19+
- [#2728](https://github.com/thanos-io/thanos/pull/2728) Query: Fixed panics when using larger number of replica labels with short series label sets.
1920

2021
### Changed
2122

‎pkg/query/iter.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,8 +381,12 @@ func (s *dedupSeriesSet) peekLset() labels.Labels {
381381
}
382382
// Check how many replica labels are present so that these are removed.
383383
var totalToRemove int
384-
for index := 0; index < len(s.replicaLabels); index++ {
385-
if _, ok := s.replicaLabels[lset[len(lset)-index-1].Name]; ok {
384+
for i := 0; i < len(s.replicaLabels); i++ {
385+
if len(lset)-i == 0 {
386+
break
387+
}
388+
389+
if _, ok := s.replicaLabels[lset[len(lset)-i-1].Name]; ok {
386390
totalToRemove++
387391
}
388392
}

‎pkg/query/querier.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -214,20 +214,17 @@ func (q *querier) Select(_ bool, hints *storage.SelectHints, ms ...*labels.Match
214214
}, warns, nil
215215
}
216216

217-
// TODO(fabxc): this could potentially pushed further down into the store API
218-
// to make true streaming possible.
217+
// TODO(fabxc): this could potentially pushed further down into the store API to make true streaming possible.
219218
sortDedupLabels(resp.seriesSet, q.replicaLabels)
220-
221219
set := &promSeriesSet{
222220
mint: q.mint,
223221
maxt: q.maxt,
224222
set: newStoreSeriesSet(resp.seriesSet),
225223
aggrs: aggrs,
226224
}
227225

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

‎pkg/query/querier_test.go‎

Lines changed: 50 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -800,123 +800,36 @@ func TestSortReplicaLabel(t *testing.T) {
800800
// 0 Single deduplication label.
801801
{
802802
input: []storepb.Series{
803-
{Labels: []storepb.Label{
804-
{Name: "a", Value: "1"},
805-
{Name: "b", Value: "replica-1"},
806-
{Name: "c", Value: "3"},
807-
}},
808-
{Labels: []storepb.Label{
809-
{Name: "a", Value: "1"},
810-
{Name: "b", Value: "replica-1"},
811-
{Name: "c", Value: "3"},
812-
{Name: "d", Value: "4"},
813-
}},
814-
{Labels: []storepb.Label{
815-
{Name: "a", Value: "1"},
816-
{Name: "b", Value: "replica-1"},
817-
{Name: "c", Value: "4"},
818-
}},
819-
{Labels: []storepb.Label{
820-
{Name: "a", Value: "1"},
821-
{Name: "b", Value: "replica-2"},
822-
{Name: "c", Value: "3"},
823-
}},
803+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "3"}}},
804+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}}},
805+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "4"}}},
806+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "c", Value: "3"}}},
824807
},
825808
exp: []storepb.Series{
826-
{Labels: []storepb.Label{
827-
{Name: "a", Value: "1"},
828-
{Name: "c", Value: "3"},
829-
{Name: "b", Value: "replica-1"},
830-
}},
831-
{Labels: []storepb.Label{
832-
{Name: "a", Value: "1"},
833-
{Name: "c", Value: "3"},
834-
{Name: "b", Value: "replica-2"},
835-
}},
836-
{Labels: []storepb.Label{
837-
{Name: "a", Value: "1"},
838-
{Name: "c", Value: "3"},
839-
{Name: "d", Value: "4"},
840-
{Name: "b", Value: "replica-1"},
841-
}},
842-
{Labels: []storepb.Label{
843-
{Name: "a", Value: "1"},
844-
{Name: "c", Value: "4"},
845-
{Name: "b", Value: "replica-1"},
846-
}},
809+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-1"}}},
810+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}}},
811+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}, {Name: "b", Value: "replica-1"}}},
812+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "4"}, {Name: "b", Value: "replica-1"}}},
847813
},
848814
dedupLabels: map[string]struct{}{"b": {}},
849815
},
850816
// 1 Multi deduplication labels.
851817
{
852818
input: []storepb.Series{
853-
{Labels: []storepb.Label{
854-
{Name: "a", Value: "1"},
855-
{Name: "b", Value: "replica-1"},
856-
{Name: "b1", Value: "replica-1"},
857-
{Name: "c", Value: "3"},
858-
}},
859-
{Labels: []storepb.Label{
860-
{Name: "a", Value: "1"},
861-
{Name: "b", Value: "replica-1"},
862-
{Name: "b1", Value: "replica-1"},
863-
{Name: "c", Value: "3"},
864-
{Name: "d", Value: "4"},
865-
}},
866-
{Labels: []storepb.Label{
867-
{Name: "a", Value: "1"},
868-
{Name: "b", Value: "replica-1"},
869-
{Name: "b1", Value: "replica-1"},
870-
{Name: "c", Value: "4"},
871-
}},
872-
{Labels: []storepb.Label{
873-
{Name: "a", Value: "1"},
874-
{Name: "b", Value: "replica-2"},
875-
{Name: "b1", Value: "replica-2"},
876-
{Name: "c", Value: "3"},
877-
}},
878-
{Labels: []storepb.Label{
879-
{Name: "a", Value: "1"},
880-
{Name: "b", Value: "replica-2"},
881-
{Name: "c", Value: "3"},
882-
}},
819+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "3"}}},
820+
{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"}}},
821+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "4"}}},
822+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "b1", Value: "replica-2"}, {Name: "c", Value: "3"}}},
823+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "c", Value: "3"}}},
883824
},
884825
exp: []storepb.Series{
885-
{Labels: []storepb.Label{
886-
{Name: "a", Value: "1"},
887-
{Name: "c", Value: "3"},
888-
{Name: "b", Value: "replica-1"},
889-
{Name: "b1", Value: "replica-1"},
890-
}},
891-
{Labels: []storepb.Label{
892-
{Name: "a", Value: "1"},
893-
{Name: "c", Value: "3"},
894-
{Name: "b", Value: "replica-2"},
895-
}},
896-
{Labels: []storepb.Label{
897-
{Name: "a", Value: "1"},
898-
{Name: "c", Value: "3"},
899-
{Name: "b", Value: "replica-2"},
900-
{Name: "b1", Value: "replica-2"},
901-
}},
902-
{Labels: []storepb.Label{
903-
{Name: "a", Value: "1"},
904-
{Name: "c", Value: "3"},
905-
{Name: "d", Value: "4"},
906-
{Name: "b", Value: "replica-1"},
907-
{Name: "b1", Value: "replica-1"},
908-
}},
909-
{Labels: []storepb.Label{
910-
{Name: "a", Value: "1"},
911-
{Name: "c", Value: "4"},
912-
{Name: "b", Value: "replica-1"},
913-
{Name: "b1", Value: "replica-1"},
914-
}},
915-
},
916-
dedupLabels: map[string]struct{}{
917-
"b": {},
918-
"b1": {},
826+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
827+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}}},
828+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}, {Name: "b1", Value: "replica-2"}}},
829+
{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"}}},
830+
{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "4"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
919831
},
832+
dedupLabels: map[string]struct{}{"b": {}, "b1": {}},
920833
},
921834
}
922835
for _, test := range tests {
@@ -1007,6 +920,36 @@ func TestDedupSeriesSet(t *testing.T) {
1007920
"replica": {},
1008921
},
1009922
},
923+
{
924+
// Regression tests against: https://github.com/thanos-io/thanos/issues/2645.
925+
// We were panicking on requests with more replica labels than overall labels in any series.
926+
input: []series{
927+
{
928+
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-1"}},
929+
samples: []sample{{10000, 1}, {20000, 2}},
930+
}, {
931+
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-2"}},
932+
samples: []sample{{60000, 3}, {70000, 4}},
933+
}, {
934+
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-3"}},
935+
samples: []sample{{100000, 10}, {150000, 20}},
936+
}, {
937+
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}},
938+
samples: []sample{{10000, 1}, {20000, 2}},
939+
},
940+
},
941+
exp: []series{
942+
{
943+
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}},
944+
samples: []sample{{10000, 1}, {20000, 2}, {60000, 3}, {70000, 4}, {100000, 10}, {150000, 20}},
945+
},
946+
{
947+
lset: labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}},
948+
samples: []sample{{10000, 1}, {20000, 2}},
949+
},
950+
},
951+
dedupLabels: map[string]struct{}{"replica": {}, "replica2": {}, "replica3": {}, "replica4": {}, "replica5": {}, "replica6": {}, "replica7": {}},
952+
},
1010953
{
1011954
// Multi dedup label.
1012955
input: []series{
@@ -1131,7 +1074,7 @@ func TestDedupSeriesSet(t *testing.T) {
11311074
},
11321075
},
11331076
{
1134-
// Same thing but not for counter should not adjust antything.
1077+
// Same thing but not for counter should not adjust anything.
11351078
isCounter: false,
11361079
input: []series{
11371080
{

0 commit comments

Comments
 (0)