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.

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

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

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Jun 5, 2020

Fixes #2645

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

// trimReplicaLabels removed replica labels from all series and re-sorts the set so that the same series are coming right after each other.
func trimReplicaLabels(set []storepb.Series, replicaLabels map[string]struct{}) {
for si := range set {
lset := set[si].Labels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name the lset to labelSet? Sounds more intuitive

Copy link
Member Author

@bwplotka bwplotka Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would stick to lset I know it might be confusing at the beginning but we are using this name everywhere in Thanos, Prometheus and Cortex. Plus we use lset ONLY for label sets (: So with this after learning that lset might make sense, what do you think? (:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense to follow the convention 😁

@bwplotka
Copy link
Member Author

bwplotka commented Jun 8, 2020

It should be good to review, just some tests I need to fix: FAIL: TestQuerier_Select/select_overlapping_data_with_partial_error/dedup=true/querier.Select (0.00s)

@bwplotka
Copy link
Member Author

Should be good to go now.

…d panic seen when using larger number of replicas and small series.

Fixes #2645


Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@bwplotka bwplotka merged commit e3afdfa into master Jun 10, 2020
@bwplotka bwplotka deleted the issue-range-2645 branch June 10, 2020 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

index out of range [-1]
3 participants