Skip to content

Support caching bucket for cleaner #6778

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

Merged
merged 3 commits into from
Jun 3, 2025

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jun 2, 2025

What this PR does:

There was an attempt before to support metadata cache for compactor in #5682.

It only supports compactor and ignores cleaner as it is intended for cleaner to be source of truth of the bucket.

This makes sense but Cleaner also does a lot of calls to object storage so it would be nice to explore if we can use metadata caching bucket for Cleaner.

This PR tries to cache certain objstore operations for cleaner:

  • GET for block meta.json
  • GET for tenant deletion marker
  • Iter for list of tenants

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@@ -245,6 +245,47 @@ func CreateCachingBucket(chunksConfig ChunksCacheConfig, metadataConfig Metadata
return storecache.NewCachingBucket(bkt, cfg, logger, reg)
}

func CreateCachingBucketForCompactor(metadataConfig MetadataCacheConfig, cleaner bool, bkt objstore.InstrumentedBucket, logger log.Logger, reg prometheus.Registerer) (objstore.InstrumentedBucket, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this and CreateCachingBucket have a inner shared logic? They seems very similar, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right they are similar. I don't see a good way to share the same function for the two. The main difference is thosecfg.CacheXXX calls can be different for read and compaction path

@danielblando
Copy link
Contributor

Should we use another config? I dont like creating a new config, but if this causes issues on cleaner we would need to turn off cache in both compactors. It would be nice to have a separate config while we are experiment with it

@yeya24
Copy link
Contributor Author

yeya24 commented Jun 2, 2025

Should we use another config? I dont like creating a new config, but if this causes issues on cleaner we would need to turn off cache in both compactors. It would be nice to have a separate config while we are experiment with it

Yeah I can create a new flag to enable this for cleaner separately. Good call out.

@danielblando
Copy link
Contributor

@alexqyle any concerns with this caches being introduced in cleaner?

@alexqyle
Copy link
Contributor

alexqyle commented Jun 3, 2025

@alexqyle any concerns with this caches being introduced in cleaner?

I think it is ok for cleaner to only cache tenant iter and metadata files

@yeya24 yeya24 force-pushed the caching-bucket-cleaner branch from 0ababce to b843a94 Compare June 3, 2025 22:16
Signed-off-by: yeya24 <[email protected]>
@yeya24 yeya24 force-pushed the caching-bucket-cleaner branch from b843a94 to 898582e Compare June 3, 2025 22:21
Signed-off-by: Ben Ye <[email protected]>
@yeya24 yeya24 force-pushed the caching-bucket-cleaner branch from a8046ff to 01ecacb Compare June 3, 2025 22:26
Copy link
Contributor

@danielblando danielblando left a comment

Choose a reason for hiding this comment

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

Nice

@yeya24 yeya24 merged commit cca0cc7 into cortexproject:master Jun 3, 2025
17 checks passed
@yeya24 yeya24 deleted the caching-bucket-cleaner branch June 3, 2025 23:01
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.

3 participants