-
Couldn't load subscription status.
- Fork 147
feat: implement hybrid cache architecture #1246
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
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1246 +/- ##
==========================================
+ Coverage 82.64% 82.66% +0.02%
==========================================
Files 70 73 +3
Lines 3007 3185 +178
Branches 501 537 +36
==========================================
+ Hits 2485 2633 +148
- Misses 419 444 +25
- Partials 103 108 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
12b56bd to
734621c
Compare
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
…zi/git-proxy into feature/hybrid-cache
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.
I'm looking forward to faster pulls!
Caches are a common place for logic or security problems so we may want added scrutiny and testing for this PR to be extra safe :)
Added some thoughts, not ran this locally just yet.
| if (process.env.NODE_ENV === 'test') { | ||
| // TEST: Full cleanup (bare cache + all working copies) | ||
| try { | ||
| if (fs.existsSync('./.remote')) { | ||
| fs.rmSync('./.remote', { recursive: true, force: true }); | ||
| step.log('Test environment: Full .remote directory cleaned'); | ||
| } else { | ||
| step.log('Test environment: .remote directory already clean'); | ||
| } | ||
| } catch (err) { | ||
| step.log(`Warning: Could not clean .remote directory: ${err}`); | ||
| } | ||
| } else { |
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.
I feel like a safer option that ensures what we're testing more closely matches prod would be to either do some separate cleanup as necessary in the tests, or adjust config to have a much lower max (or even 0) then we can see if CacheManager itself is doing the cleanup.
| // Sort repositories by last accessed (oldest first for removal) | ||
| const reposToEvaluate = [...stats.repositories].sort( | ||
| (a, b) => a.lastAccessed.getTime() - b.lastAccessed.getTime(), | ||
| ); |
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.
Nice
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.
I can't remember if we can use toSorted yet. If we still cant a comment to use toSorted in the future would help to find this later :)
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.
I think it should be available from node 20 so yes I'll use it
| return 0; | ||
| } | ||
|
|
||
| return Math.round(totalBytes / (1024 * 1024)); // Convert to MB |
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.
| return Math.round(totalBytes / (1024 * 1024)); // Convert to MB | |
| return Math.ceil(totalBytes / (1024 * 1024)); // Convert to MB |
rounding down would hide some amount of KB up to 511KB. Not the end of the world but safest to always round up rather than be shortchanged.
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.
There's no conversion anymore since we are using bytes everywhere
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.
Just a code review for now - I'll be testing the code later!
I was wondering if you could do some profiling to verify/document the speed improvements with this PR. Would be nice to try it out with massive repos (for example https://github.com/backstage/backstage which is >10GB) to see the difference.
I'll try to do some profiling soon as well 🙂
| } | ||
|
|
||
| export class CacheManager { | ||
| private cacheDir: string; |
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.
Is this cacheDir related to the cachedir library in package.json or the cacheDir used in the ConfigLoader? If not, we might want to pick a different name to disambiguate them.
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.
I'm wondering what would happen if two pulls happen simultaneously and execute touchRepository or enforceLimits at the same time - wouldn't this cause some kind of race condition? Especially with cache statistics in the mix, we wouldn't be able to accurately pick up the latest accessed repository.
| let freedMB = 0; | ||
|
|
||
| // Sort repositories by last accessed (oldest first for removal) | ||
| const reposToEvaluate = [...stats.repositories].sort( |
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.
Is there a reason to sort the repositories twice? The first sort happens inside the getCacheStats() call right above.
| /** | ||
| * Calculate directory size in MB | ||
| */ | ||
| private getDirectorySize(dirPath: string): number { |
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.
Should the error clauses here be logging something instead of failing silently? Such as when there's an error on calculateSize(dirPath).
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.
Would be nice to have a short README on how the pullRemote works and what the hybrid cache architecture is doing. This'll make things much clearer for future development and finding potential flaws in our logic 🙂
|
why on earth is backstage's repo 10GB? 😭 |
|
@06kellyjac It might be because of the tons of plugins that come with the repo (many of which have redundant components). Pulling with |
Issue: #1207
Summary
Two-tier cache system for faster git operations: persistent bare repos + ephemeral working copies.
Architecture
.remote/cache/- Shared bare repositories (persistent).remote/work/<push-id>/- Per-push working copies (temporary)Configuration
Add to
proxy.config.json:{ "cache": { "maxSizeGB": 2, "maxRepositories": 50, "cacheDir": "./.remote/cache" } }Security