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

[WIP] Remove useless cache locks in LRUs #13794

Draft
wants to merge 33 commits into
base: develop
Choose a base branch
from

Conversation

v4lproik
Copy link
Contributor

@v4lproik v4lproik commented Mar 24, 2024

What type of PR is this?
Enhancement
Bug fix

What does this PR do? Why is it needed?

  • This PR does several things:
  • Update the hashicorp LRU cache to the latest stable version
  • Implement a better contract between caches / callers allowing homogenous code to be reused across the different types of cache (miss/hit prom metrics to be consistent, same behaviour when value not found in cache, have the same controls when inserting into the cache eg. nil value are not allowed... etc)
  • Fix some bugs along the way

Which issues(s) does this PR fix?
#13723

Other notes for review

@v4lproik v4lproik changed the title [WIP] Remove useless cache locks [WIP] Remove useless cache locks in LRUs Mar 25, 2024
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

As a general rule, it would be much easier to review these changes if you split it into different PRs by each specific cache. Some of these are on critical paths, so being able to review them independently is important. Also in the event any change breaks something we can simply revert the PR to the specific cache rather than for all the caches.

@v4lproik
Copy link
Contributor Author

You are absolutely correct. I have kept this PR as a work in progress because it has grown larger than what is typically acceptable, as you pointed out.

However, I have kept it for a few reasons:

  • Firstly, I am consistently testing the changes locally to ensure that I am not inadvertently breaking anything at runtime or having odd results/errors.
  • Secondly, I wanted to ensure that implementing lru/v2 at a low level with generics for each cache using the new interface would be feasible.

I have broken down the commits per service to make it easier for me to cherry-pick the changes on a per-PR basis. Could you please indicate which cache you would like to see this implementation for in the first review as I am nearly finished?

Thank you!

@nisdas
Copy link
Member

nisdas commented Mar 26, 2024

@v4lproik I have no preference on the order of caches to review. Feel free to open PRs in any order you would like

@v4lproik
Copy link
Contributor Author

@nisdas I have opened the rework of the Committees Cache if you want to have a look at it.

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.

None yet

2 participants