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

perf(bloomstore): Cache metas LIST operation #12414

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Mar 31, 2024

What this PR does / why we need it:

Reduce the amount of class A list operations on object storage.

@chaudum chaudum force-pushed the chaudum/cache-metas-list-op branch from 69a7894 to d7f002c Compare April 2, 2024 08:06
@chaudum chaudum marked this pull request as ready for review April 2, 2024 08:25
@chaudum chaudum requested a review from a team as a code owner April 2, 2024 08:25
return cached.objects, cached.prefixes, nil
}

c.mtx.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be moved after the c.ObjectClient.List? Right now it's blocking any List of till the inner client finished.
IIUC we might want to do this so two lists doesn't happen at the same time but I think that's not ideal. Would make sense to block prevent two simultaneus calls for the same prefix tho.

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, I would like to avoid multiple concurrent LIST calls. You still need to sync the map access thereafter. So I think it makes sense to lock the whole operation.

Having separate locks for different prefixes makes eviction more complex. Not sure if that's worth it then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. We can re-evaluate if we see lock contention.

@chaudum chaudum force-pushed the chaudum/cache-metas-list-op branch from d7f002c to 86af424 Compare April 2, 2024 09:27
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum force-pushed the chaudum/cache-metas-list-op branch from 86af424 to e9df2e3 Compare April 2, 2024 18:56
@chaudum chaudum enabled auto-merge (squash) April 2, 2024 18:57
@chaudum chaudum merged commit bf0dd9d into main Apr 3, 2024
10 checks passed
@chaudum chaudum deleted the chaudum/cache-metas-list-op branch April 3, 2024 06:49
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
Reduce the amount of class A list operations on object storage.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants