Refactor in preparation for adding more sophisticated cache cleanup policies #461
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the cache cleanup code to prepare for more sophisticated cleanup policies, particularly threshold-based eviction. The refactoring consolidates cleanup logic and introduces a new config field for aggressive cleanup's lower threshold, while maintaining existing TTI/TTL-based cleanup behavior.
Key Changes:
- Renamed
scantocleanupto better reflect its purpose and added support for custom policy-based cleanup - Refactored
checkAggressiveCleanuptoshouldAggroto return a boolean instead of selecting TTL values - Added
AggressiveLowerThresholdconfig field to support future threshold-based eviction
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/store/cleanup.go | Refactored cleanup logic to separate TTL-based and custom policy-based cleanup, added new config field, and simplified aggressive cleanup detection |
| lib/store/cleanup_test.go | Updated tests to use new CleanupConfig struct and renamed method calls from scan to cleanup and checkAggressiveCleanup to shouldAggro |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sambhav-jain-16
left a comment
There was a problem hiding this comment.
I have some comments, my major comment is leave out the custom policy as todo rather than having the code in this refactor
There was a problem hiding this comment.
It was decided that #462 will be merged very quickly with this. Hence, I'm ignoring the nit fixes and TODO comments.
I went over the PR twice and looks like there is no regression expected atleast from the code side. That's why LGTM !
4905f22 to
82a9da8
Compare
This PR is non-functional and only refactors the store cache cleanup code to prepare for future PRs, where I will add support for more sophisticated cache cleanup policies for the origin and build-index's caches, where blobs and tags are stored on disk.
The disk caches (where blobs and tags are cached upon download) are cleaned up with a basic TTI + TTL config. Additionally, an aggressive utilization % threshold can be specified, above which a special "aggressive" cleanup config is applied, with much more aggressive TTL values.
However, this usually leads to the cache util dropping from dangerously high to suboptimally low, e.g. 85% -> 20%. During high load, this is suboptimal, as it causes the origin to redownload files from its storage backend (GCS/S3), which it could have otherwise not deleted, while still being safe from disk exhaustion. Ideally, the disk would drop from 85% -> 50%. This is called "threshold-based eviction" and this PR refactors the clean up code to prepare for adding support for it in a future PR.
Additionally, a custom policy will be supported, where instead of a TTI + TTL based cache cleanup policy, a custom prioritization order between blobs will be added, which will delete blobs until the lower threshold is added.