-
Couldn't load subscription status.
- Fork 24
fix: fixes race conditions in EntitlementPolicyCache #2657
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
base: main
Are you sure you want to change the base?
Conversation
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imdominicreed this feels pretty aggressive. By shifting the cache to in memory managed by policy it removes the centralized control which will have long term impact on admins from managing cache behavior across the platform.
It is true that ristretto is eventually consistent so the first few hits might be fetching from the DB until the cache is available, but this is expected. If you're encountering some unexpected behavior I'd prefer to address the defect rather than tossing the baby with the bathwater.
I think its important to consider the eviction case, but I don't think its realistic that the policy would be too large at this point unless the admin uses an extremely small cache size.
One thing we're trying to set ourselves up for is support for other cache solutions such as distributive cache via Redis. The cache manager uses https://github.com/eko/gocache so that it will be possible to give admins the option to choose the cache layer they want.
Proposed Changes
ristretto is eventually consistent.. causing a race condition on the first request, updates cache, fetches from cache, nothings there. using a cache here doesn't seem right since we are pulling these in anyways. if the policy is too big that it gets evicted from the cache then what is the cache doing.
This PR removes the ristretto cache, and just stores the entitlement policy. also, sync is added on the read write operations.
Checklist
Testing Instructions