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

ref: Clean up empty cache directories #887

Merged
merged 4 commits into from
Oct 4, 2022
Merged

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Oct 4, 2022

Thus far, the cleanup process never removed empty cache directories. For one, the worklist based approach cleaning the parent before the children, did not make this possible in the first place, but also because normal operations would have errored out on failing FS operations, for example when a directory or file could not be created in a non-existent directory.

This changes the cleanup process to be depth-first, cleaning up empty directories as it goes. It also makes the writing side of things extra defensive and retries FS operations in the unlikely case that a directory is being removed while operating in it.

Thus far, the cleanup process never removed empty cache directories.
For one, the worklist based approach cleaning the parent before the children,
did not make this possible in the first place, but also because normal
operations would have errored out on failing FS operations, for example
when a directory or file could not be created in a non-existent directory.

This changes the cleanup process to be depth-first, cleaning up empty
directories as it goes. It also makes the writing side of things extra
defensive and retries FS operations in the unlikely case that a directory
is being removed while operating in it.
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2022

Codecov Report

Base: 70.27% // Head: 70.05% // Decreases project coverage by -0.21% ⚠️

Coverage data is based on head (9a28d56) compared to base (3b0bbdf).
Patch coverage: 45.83% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
- Coverage   70.27%   70.05%   -0.22%     
==========================================
  Files          58       58              
  Lines       11311    11382      +71     
==========================================
+ Hits         7949     7974      +25     
- Misses       3362     3408      +46     
Impacted Files Coverage Δ
crates/symbolicator/src/services/cacher.rs 79.43% <35.48%> (-3.28%) ⬇️
crates/symbolicator/src/cache.rs 94.02% <50.76%> (-2.53%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

crates/symbolicator/src/cache.rs Outdated Show resolved Hide resolved
crates/symbolicator/src/cache.rs Outdated Show resolved Hide resolved
crates/symbolicator/src/cache.rs Show resolved Hide resolved
crates/symbolicator/src/cache.rs Outdated Show resolved Hide resolved
crates/symbolicator/src/services/cacher.rs Outdated Show resolved Hide resolved
@Swatinem Swatinem marked this pull request as ready for review October 4, 2022 12:47
@Swatinem Swatinem requested a review from a team October 4, 2022 12:47
crates/symbolicator/src/cache.rs Outdated Show resolved Hide resolved
@Swatinem Swatinem merged commit b03830b into master Oct 4, 2022
@Swatinem Swatinem deleted the release/cleanup-dirs branch October 4, 2022 13:29
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.

4 participants