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

Add counter for chunks from ingesters entirely deduplicated #2713

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Aug 12, 2022

What this PR does

Add a counter for tracking the number of chunks that could be entirely discarded
as duplicates of an existing chunk when querying ingesters. This is a requirement
of testing distributor <-> ingester timeout changes.

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@github-actions

This comment has been minimized.

@56quarters 56quarters force-pushed the 56quarters/measure-chunk-dedupe branch from d840bc3 to 3221c3b Compare August 12, 2022 16:28
@github-actions

This comment has been minimized.

@56quarters 56quarters marked this pull request as ready for review August 12, 2022 17:08
@56quarters 56quarters force-pushed the 56quarters/measure-chunk-dedupe branch from 3221c3b to a1266f6 Compare August 12, 2022 19:54
Add a counter for tracking the number of chunks that could be entirely discarded
as duplicates of an existing chunk when querying ingesters. This is a requirement
of testing distributor <-> ingester timeout changes.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters force-pushed the 56quarters/measure-chunk-dedupe branch from a1266f6 to 7f19509 Compare August 12, 2022 19:54
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Change looks good to me. It would be nice to have a unit test for this change too.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Copy link
Contributor Author

Change looks good to me. It would be nice to have a unit test for this change too.

I'm struggling to come up with a good way to test this without a lot of refactoring. Because this runs as an operation against the ring, it seems that the operation returns as soon as the required number of ingesters have responded. I've tried with 3 ingesters and RF=3 as well as 2 ingesters and RF=2.

@pstibrany
Copy link
Member

I'm struggling to come up with a good way to test this without a lot of refactoring. Because this runs as an operation against the ring, it seems that the operation returns as soon as the required number of ingesters have responded. I've tried with 3 ingesters and RF=3 as well as 2 ingesters and RF=2.

Perhaps adding a check into some integration test would be much simpler -- just see if the metric has expected value. But feel free to not block on the test.

@56quarters 56quarters merged commit 8a13ad4 into main Aug 15, 2022
@56quarters 56quarters deleted the 56quarters/measure-chunk-dedupe branch August 15, 2022 16:40
56quarters added a commit that referenced this pull request Aug 16, 2022
Add a counter for tracking the number of chunks that could be entirely discarded
as duplicates of an existing chunk when querying ingesters. This is a requirement
of testing distributor <-> ingester timeout changes.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this pull request Aug 16, 2022
…2744)

Add a counter for tracking the number of chunks that could be entirely discarded
as duplicates of an existing chunk when querying ingesters. This is a requirement
of testing distributor <-> ingester timeout changes.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
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.

2 participants