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

feat: add cache pruning #386

Merged
merged 1 commit into from
Jun 23, 2018
Merged

feat: add cache pruning #386

merged 1 commit into from
Jun 23, 2018

Conversation

mzgoddard
Copy link
Owner

@mzgoddard mzgoddard commented Jun 22, 2018

Fix #84

Prune, by default, caches that are older than 2 days after accumulating 50 megabytes of cache.

@mzgoddard mzgoddard force-pushed the prune-caches branch 2 times, most recently from f946d68 to b3eb8e9 Compare June 22, 2018 14:35
README.md Outdated
// Caches created or modified before `deleteAfter` are not considered for
// deletion. They must be at least this (default: 2 days) old in
// milliseconds.
deleteAfter: 2 * 24 * 60 * 60 * 1000,
Copy link

Choose a reason for hiding this comment

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

maybe options:

cachePrune.maxAge
cachePrune.sizeThreshold

cachePrune vs prune. is like "whatever" this is basically just a cache anyway...

@mzgoddard mzgoddard force-pushed the prune-caches branch 5 times, most recently from 92f5a39 to b2ac142 Compare June 22, 2018 15:15
Copy link

@MattSurabian MattSurabian left a comment

Choose a reason for hiding this comment

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

Pretty exciting to see this feature get added! Nice one

README.md Outdated
maxAge: 2 * 24 * 60 * 60 * 1000,
// All caches together must be larger than `sizeThreshold` before any
// caches will be deleted. Together they must be at least this
// (default: 50 MB) in big in bytes.

Choose a reason for hiding this comment

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

extra "in"


static async fromDirectory(dir) {
const info = new CacheInfo(basename(dir));
info.lastModified = +(await stat(join(dir, 'stamp'))).mtime;

Choose a reason for hiding this comment

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

Why do you need the + here? Isn't this returning a date string?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's a Date object. And the + is implicitly casting it into a Number. I should do better and use Number(...) or .getTime()

);

for (const info of oldInfos) {
rimraf(join(this.cacheRoot, info.id));

Choose a reason for hiding this comment

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

Not sure the state on rimraf. The build is failing and there's an ignored issue from a month ago with folks wondering what's up with the state of the project. Since you're not really using any of the more "advanced" features of rimraf maybe it's not a dep you need?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting. Looks like the build fail is use of const and rimraf 9 months ago was still testing for node 0.10 and 0.12 support which don't support const. hard-source is supporting back to node 8 so I'm not worried about old node incompatibility.

I'm not sure if I want to replace rimraf. hard-source has been using the current version since December (2017). https://www.npmjs.com/search?q=keywords%3Arm is a fun search to look for alternatives though.

- Add `prune` configuration information to README

Prune, by default, caches that are older than 2 days after accumulating
50 megabytes of cache.
@mzgoddard mzgoddard merged commit c685a61 into master Jun 23, 2018
@mzgoddard mzgoddard deleted the prune-caches branch June 23, 2018 02:23
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

3 participants