Alternative implementation: Push replica labels at the end after Recv #5742

fpetkovski · 2022-09-29T12:33:07Z

This commit removes the case for a global sort on all series in the querier, and redistributes in places where series are received from stores.

This is an alternative implementation to #5692.

I ran a query with this implementation against 100K series and the execution time dropped from 26s to 19s compared to main. With the new engine, the query took 9s.

Signed-off-by: Filip Petkovski filip.petkovsky@gmail.com

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Verification

GiedriusS · 2022-09-30T08:18:58Z

pkg/store/proxy_heap.go

+}
+
+func (m *dedupReadyResponse) LessWithoutReplicaLabels(other *dedupReadyResponse) bool {
+	if m.GetSeries() != nil && other.GetSeries() != nil {


It crashes for me here. Generated code looks like this:

if x, ok := m.GetResult().(*SeriesResponse_Series); ok { return x.Series }

It doesn't check whether x is not nil :'/ Maybe we can add a small helper:

func (m *SeriesResponse) GetSeriesNotNil() *Series { if x, ok := m.GetResult().(*SeriesResponse_Series); ok { if x == nil { return nil } return x.Series } return nil }

The problem might be somewhere else, though, that we are comparing nil with not nil. Maybe related to #5717?

See my comment there.

Thanks for testing this. I assume you are using the lazy strategy when it crashes?

fpetkovski · 2022-10-01T09:42:52Z

pkg/store/proxy_heap.go

-		if labels.Compare(labelpb.ZLabelsToPromLabels(lbls), labelpb.ZLabelsToPromLabels(lastLbls)) == 0 {
+		// Unless a response comes from query pushdown, it is safe to merge
+		// chunks from replica series together into one slice.
+		labelsEqual := labels.Compare(labelpb.ZLabelsToPromLabels(lbls), labelpb.ZLabelsToPromLabels(lastLbls)) == 0


I would love to use labelsEqual = resp.signature == lastResponse.signature here, but unfortunately it breaks tests with overlapping and interleaved chunks from prometheus replicas. This was introduced and solved in:

querier: Rate over deduplicated counter from many replicas can lead to double reset account. #2401

Added LocalStore and realistic data for querier counter reset bug. #2522

I tested that change in a canary environment, and together with the new engine saw a decrease in a query from 75s to 8s, which is just insane. We have a layered querier setup, so deduplicating as soon as possible makes a massive difference.

GiedriusS · 2022-10-03T15:18:11Z

pkg/store/proxy.go

@@ -342,6 +351,12 @@ func (s *ProxyStore) Series(originalRequest *storepb.SeriesRequest, srv storepb.
 		}
 	}

+	if respHeap.UnsortedSetDetected() {


This feels a bit like implementing InfoServer through the Series service. What do you think about checking st.SendsSortedSeries() instead and if all are true then we could also advertise that SendsSortedSeries() == true in Thanos Query. And if there is at least one where SendsSortedSeries() == false then let's also advertise false in Thanos Query.

I have been playing around with your PR a little bit and ☝️ seems like a much cleaner solution. I can try opening up a PR to it tomorrow or on Wednesday.

I think this would make sense, I'll rework the PR to change the detection mechanism.
However, should we also change stores to have a sorted detection without replica labels? This should be possible now since replica labels are part of the Series request.

Now that I read that I again, with the current implementation of stores, I am not sure if it is possible to make a detection statically without inspecting the data. A user can specify any label as the replica label in a Query component. So stores need to resort data arbitrarily in the Series call if we want to have checks through info calls.

If we want to remove the sort in the querier, data needs to be sorted without replica labels for k-way merge to work. That means data needs to be resorted in every store that can return multiple replicas of the same series. Since users can specify any label as the replica label, this in practice means each store should be able to resort data.

Taking the following example

1. a=1, r=1, z=1 2. a=1, r=1, z=2 3. a=1, r=2, z=1

if the replica label is r, data needs to be sent in the following order for deduplication to work

1. a=1, z=1, r=1 3. a=1, z=2, r=1 2. a=1, z=1, r=2

So rows 2. and 3. need to be switched.

bwplotka · 2022-10-03T19:53:29Z

Panics on my side:

bwplotka · 2022-10-03T19:53:37Z

let's see tomorrow (:

GiedriusS · 2022-10-04T07:03:21Z

Panics on my side:

You need to merge the latest main into this branch to fix this 😄

fpetkovski · 2022-10-04T07:11:58Z

Might also be fixed by thanos-io/promql-engine#55

This commit removes the case for a global sort on all series in the querier, and redistributes in places where series are received from stores. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

stale · 2022-11-12T12:31:17Z

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

GiedriusS · 2023-02-20T14:17:57Z

I think we can close this since an alternative implementation was merged for solving this problem? 😄

pull-request-size bot added the size/XXL label Sep 29, 2022

fpetkovski changed the title ~~Push replica labels at the end after Recv~~ Alternative implementation: Push replica labels at the end after Recv Sep 29, 2022

fpetkovski mentioned this pull request Sep 29, 2022

Push replica labels at the end after Recv #5692

Closed

2 tasks

fpetkovski force-pushed the remove-sort branch from 50536a0 to 716e4b9 Compare September 29, 2022 13:52

pull-request-size bot added size/XL and removed size/XXL labels Sep 29, 2022

GiedriusS reviewed Sep 30, 2022

View reviewed changes

GiedriusS mentioned this pull request Sep 30, 2022

store: fix nil panic in proxy heap #5746

Merged

fpetkovski force-pushed the remove-sort branch 2 times, most recently from 4f12269 to 83647d8 Compare October 1, 2022 05:39

fpetkovski commented Oct 1, 2022

View reviewed changes

GiedriusS reviewed Oct 3, 2022

View reviewed changes

fpetkovski force-pushed the remove-sort branch from 83647d8 to b804880 Compare October 4, 2022 16:46

fpetkovski added 2 commits October 10, 2022 10:42

Push replica labels at the end after Recv

057055c

This commit removes the case for a global sort on all series in the querier, and redistributes in places where series are received from stores. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

Remove unnecessary changes

4b8d644

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the remove-sort branch from b804880 to 4b8d644 Compare October 10, 2022 08:42

Rebase against main

f8a65d0

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

fpetkovski mentioned this pull request Oct 17, 2022

Implement dedup-ready label sorting in stores #5796

Closed

2 tasks

stale bot added the stale label Nov 12, 2022

GiedriusS closed this Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative implementation: Push replica labels at the end after Recv #5742

Alternative implementation: Push replica labels at the end after Recv #5742

fpetkovski commented Sep 29, 2022 •

edited

Loading

GiedriusS Sep 30, 2022

fpetkovski Sep 30, 2022

fpetkovski Oct 1, 2022

fpetkovski Oct 1, 2022

bwplotka Oct 3, 2022

GiedriusS Oct 3, 2022

fpetkovski Oct 3, 2022

fpetkovski Oct 3, 2022 •

edited

Loading

bwplotka commented Oct 3, 2022

bwplotka commented Oct 3, 2022

GiedriusS commented Oct 4, 2022

fpetkovski commented Oct 4, 2022

stale bot commented Nov 12, 2022

GiedriusS commented Feb 20, 2023

Alternative implementation: Push replica labels at the end after Recv #5742

Alternative implementation: Push replica labels at the end after Recv #5742

Conversation

fpetkovski commented Sep 29, 2022 • edited Loading

Changes

Verification

GiedriusS Sep 30, 2022

Choose a reason for hiding this comment

fpetkovski Sep 30, 2022

Choose a reason for hiding this comment

fpetkovski Oct 1, 2022

Choose a reason for hiding this comment

fpetkovski Oct 1, 2022

Choose a reason for hiding this comment

bwplotka Oct 3, 2022

Choose a reason for hiding this comment

GiedriusS Oct 3, 2022

Choose a reason for hiding this comment

fpetkovski Oct 3, 2022

Choose a reason for hiding this comment

fpetkovski Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

bwplotka commented Oct 3, 2022

bwplotka commented Oct 3, 2022

GiedriusS commented Oct 4, 2022

fpetkovski commented Oct 4, 2022

stale bot commented Nov 12, 2022

GiedriusS commented Feb 20, 2023

fpetkovski commented Sep 29, 2022 •

edited

Loading

fpetkovski Oct 3, 2022 •

edited

Loading