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

Improve http.send cache cleaning #5320

Closed
lukyer opened this issue Oct 27, 2022 · 13 comments · Fixed by #6392
Closed

Improve http.send cache cleaning #5320

lukyer opened this issue Oct 27, 2022 · 13 comments · Fixed by #6392

Comments

@lukyer
Copy link

lukyer commented Oct 27, 2022

Possibly related: #5300

What is the underlying problem you're trying to solve?

We store a lot (thousands) http.send responses with low TTL (10s). Currently it is unnecessarily hanging in the cache until caching.inter_query_builtin_cache.max_size_bytes is reached. When it is, only minimum items are wiped to get the cache size under the bytes limit and items are wiped only in FIFO manner instead of proactively wiping cache items with respect to the TTL.

Describe the ideal solution

Cache items are wiped and memory is freed not only when the cache is full but also when max-age (and others) is reached.

Describe a "Good Enough" solution

Periodically running goroutine wiping stale items from the cache.

@ashutosh-narkar
Copy link
Member

The other optimization we could think about is to start removing items from the cache when it hits a user-defined threshold. For example, when the cache is 80% full, start removing items from it before inserting new ones. By default, the threshold would be 100% (ie. remove items once cache is full).

@stale
Copy link

stale bot commented Dec 14, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Dec 14, 2022
@lukyer
Copy link
Author

lukyer commented Dec 15, 2022

this is still valid issue, we are waiting for fix 👍

@stale stale bot removed the inactive label Dec 15, 2022
@anderseknert
Copy link
Member

@lukyer that an issue becomes inactive does not mean it is invalid, only that there are no immediate plans to work on it — unless someone steps up to do that, of course :)

@lukyer
Copy link
Author

lukyer commented Jan 10, 2023

any updates? I believe this should be enough to fix our memory leaks here: #5381

@stale
Copy link

stale bot commented Feb 9, 2023

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Feb 9, 2023
@stale stale bot removed the inactive label Mar 8, 2023
@stale
Copy link

stale bot commented Apr 7, 2023

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Apr 7, 2023
@rudrakhp
Copy link
Contributor

rudrakhp commented Nov 4, 2023

@ashutosh-narkar I believe this issue is still open? Do correct me as I could not find a related option in the documentation yet. We see a need for this as well.
IMO user should have two options to configure, the cache eviction routine will trigger if any of these events occur:

  • caching.inter_query_builtin_cache.eviction_threshold_size_percentage: X% of max_size_bytes (only valid if max_size_bytes is set since it's optional, 100% by default)
  • caching.inter_query_builtin_cache.eviction_period_seconds: X seconds since last eviction (always valid, 0 by default so never triggers)

The cache eviction routine should clean "all stale entries" and then some in FIFO fashion if required to bring it below X% in each run.
Please let me know if it sounds good and I will take a shot at contributing here. Thanks 🙂

@stale stale bot removed the inactive label Nov 4, 2023
@ashutosh-narkar
Copy link
Member

@rudrakhp feel free to look into this. What has been laid out here seems reasonable.

@rudrakhp
Copy link
Contributor

rudrakhp commented Nov 9, 2023

@ashutosh-narkar raised a initial draft of PR for this issue, please do review when you get a chance. Thanks 🙇

@rudrakhp
Copy link
Contributor

@ashutosh-narkar bumping this up. Please do review the updated PR when you have time. Thanks!

@ashutosh-narkar
Copy link
Member

ashutosh-narkar commented Dec 1, 2023

@rudrakhp I'm currently out but I'll tag other maintainers to review in the meantime. Thanks for your patience.

Copy link

stale bot commented Jan 1, 2024

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

@stale stale bot added the inactive label Jan 1, 2024
rudrakhp added a commit to rudrakhp/opa that referenced this issue Jan 9, 2024
Regularly clean up of cache entries that have expired for a more efficient use of memory.
Introduce two new parameters to tune clean up frequency and threshold for forced FIFO eviction.

Fixes open-policy-agent#5320

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
ashutosh-narkar pushed a commit that referenced this issue Jan 9, 2024
Regularly clean up of cache entries that have expired for a more efficient use of memory.
Introduce two new parameters to tune clean up frequency and threshold for forced FIFO eviction.

Fixes #5320

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

Successfully merging a pull request may close this issue.

4 participants