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

compact+store+tools: Filter for duplicate block data only within a compaction group and in parallel #4963

Merged
merged 21 commits into from
Jan 28, 2022

Conversation

nberkley
Copy link
Contributor

@nberkley nberkley commented Dec 16, 2021

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

Changes

Changes DeduplicateFilter to only look for duplicate block data within a single compaction group. This both provides a speedup by a factor of the number of group and allows the deduplication to be parallelized over groups. Concurrency level is taken from block-meta-fetch-concurrency as it is for other filters. In our deployment this dropped meta fetch time in the compactor from 10 hours to 6 minutes.

This does mean that, if you're using a replica label in the compactor, stores may load unnecessary blocks since they won't spot label change that will put the duplicate data in two different streams. This resolves after ignore-deletion-marks-delay when the store acknowledges that the old blocks were deleted. This seems like a good tradeoff since it only impacts a small section of data and provides a massive speedup for large buckets. It could be resolved by making the store aware of replica labels, but this diff is already getting rather large.

Also moves group key computation from DefaultGroupKey in the compact package to GroupKey on the Thanos object in the metadata package as now both the compact and block packages need to compute these.

Verification

local tests pass, and we're actively using a version of the compactor with this change (though based off the 0.22 release, before the rebase)

…ompaction groups and in parallel.

Signed-off-by: Nathan Berkley <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>
@nberkley
Copy link
Contributor Author

As far as I can tell the e2e test failures here are not related to my changes. Those tests pass consistently when they're run in isolation locally (full runs time out, but so does a clean checkout of main).

…ctions groups during deduplication

Signed-off-by: Nathan Berkley <[email protected]>
…els before deduplication.

Signed-off-by: Nathan Berkley <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>
@hanjm
Copy link
Member

hanjm commented Dec 26, 2021

Awesome performance improvement.🚀

@hanjm
Copy link
Member

hanjm commented Jan 5, 2022

ping @yeya24

@nberkley
Copy link
Contributor Author

@bwplotka You might care about this one (you did the review for the PR that introduced this logic)

cmd/thanos/compact.go Outdated Show resolved Hide resolved
cmd/thanos/tools_bucket.go Show resolved Hide resolved
cmd/thanos/tools_bucket.go Show resolved Hide resolved
pkg/block/fetcher.go Show resolved Hide resolved
@GiedriusS
Copy link
Member

Seems like CI failure is related.

@nberkley
Copy link
Contributor Author

Yeah, someone added some new calls to NewStoreGW on main since last run. I'll have a fixed version up in a few minutes.

@nberkley
Copy link
Contributor Author

Should be set now if someone can approve running tests.

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

yeya24
yeya24 previously approved these changes Jan 15, 2022
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

@wiardvanrij
Copy link
Member

I only did a basic check, I would recommend already adding the Changelog so this can move forward asap :)

@nberkley
Copy link
Contributor Author

I only did a basic check, I would recommend already adding the Changelog so this can move forward asap :)

I thought the guideline was changelog only for user impacting changes? Happy to add one if that's not correct.

@wiardvanrij
Copy link
Member

I thought the guideline was changelog only for user impacting changes? Happy to add one if that's not correct.

Hm I can imagine that is listed somewhere (if you know where, please give me a link :) )
However the top of the readme states

All notable changes to this project will be documented in this file

So, just from my perspective, I think it adds value to add this change to the readme. Let's say I have an edge-case when upgrading my Thanos version. I will search for changes related to my issue/component. It's nice to see this in the changelog.

Basically any change in components can have user impact, if something isn't correct ;p (I know, maybe a lame answer but it has nothing to do with the quality here, just in general!)

@nberkley
Copy link
Contributor Author

Hm I can imagine that is listed somewhere (if you know where, please give me a link :) )

From the Changelog and Review Procedure in CONTRIBUTING.md:

If your change affects users (adds or removes feature) consider adding the item to the CHANGELOG.

Also the PR template.

But point taken. I'll add a changelog entry. Better to over communicate than under communicate.

bwplotka
bwplotka previously approved these changes Jan 20, 2022
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Great job! Just minor nits, otherwise LGTM!

Thanks and sorry for delay. 🤗

pkg/block/fetcher.go Show resolved Hide resolved
str, err := e2ethanos.NewStoreGW(e, "1", svcConfig, "")

// Crank down the deletion mark delay since deduplication can miss blocks in the presence of replica labels it doesn't know about.
str, err := e2ethanos.NewStoreGW(e, "1", svcConfig, "", []string{"--ignore-deletion-marks-delay=2s"})
Copy link
Member

Choose a reason for hiding this comment

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

Do we now depend on this test to run faster than 2s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it now cannot run faster than 2s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be more clear, this is working around the bit mentioned in the description where store gateways load duplicates for a bit in the presence of replica labels. It cranks the window during which we ignore deletion marks down so that the test can pass once we recognize the deletion marks.

pkg/block/fetcher.go Show resolved Hide resolved
pkg/block/fetcher.go Outdated Show resolved Hide resolved
pkg/block/fetcher.go Outdated Show resolved Hide resolved
Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>
@nberkley
Copy link
Contributor Author

@bwplotka Can you take another look? Think all the nits are addressed.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Solid work! Thank you 💪

@GiedriusS GiedriusS merged commit 78e923d into thanos-io:main Jan 28, 2022
Nicholaswang pushed a commit to Nicholaswang/thanos that referenced this pull request Mar 6, 2022
…mpaction group and in parallel (thanos-io#4963)

* Move group key function to the metadata package. Deduplicate within compaction groups and in parallel.

Signed-off-by: Nathan Berkley <[email protected]>

* cleanup after rebase

Signed-off-by: Nathan Berkley <[email protected]>

* formatting

Signed-off-by: Nathan Berkley <[email protected]>

* Reduce mutex operations in DeduplicateFilter

Signed-off-by: Nathan Berkley <[email protected]>

* fix tests after last update from main

Signed-off-by: Nathan Berkley <[email protected]>

* formatting

Signed-off-by: Nathan Berkley <[email protected]>

* Move fetcher modifiers before filters so we can compute correct compactions groups during deduplication

Signed-off-by: Nathan Berkley <[email protected]>

* Merge the notion of filters and modifiers so we can strip replica labels before deduplication.

Signed-off-by: Nathan Berkley <[email protected]>

* formatting

Signed-off-by: Nathan Berkley <[email protected]>

* fix tests

Signed-off-by: Nathan Berkley <[email protected]>

* remove duplicate filter list entry

Signed-off-by: Nathan Berkley <[email protected]>

* FIx new calls to NewStoreGW

Signed-off-by: Nathan Berkley <[email protected]>

* add CHANGELOG

Signed-off-by: Nathan Berkley <[email protected]>

* Apply suggestions from code review

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Nathan Berkley <[email protected]>

* comment suggested in review

Signed-off-by: Nathan Berkley <[email protected]>

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Nicholaswang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants