-
Notifications
You must be signed in to change notification settings - Fork 132
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
Calculate the database size from all files in the directory #1304
Conversation
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.
This is coming along nicely, but will need some adjustements.
See the inline comments.
Also please change the commit message: Calculate the database size from all files in the directory
or something similar.
glean-core/src/database/mod.rs
Outdated
/// | ||
/// # Arguments | ||
/// | ||
/// -`path` - The path to the directory |
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.
/// -`path` - The path to the directory | |
/// * `path` - The path to the directory |
glean-core/src/database/mod.rs
Outdated
if let Ok(entry) = entry { | ||
let path = entry.path(); | ||
// the getter is returning the u64 value from NonZeroU64 | ||
total_size += file_size(&path)?.get(); |
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.
We can savae some work here. You can read the metadata
of an entry directly, that one then has the len
. You can then also remove the file_size
function as its not used anymore.
If there's no metadata for whatever reason, we shouldn't bail out early here. If we can't read the file size of a single file, we should still read the remaining files.
glean-core/src/database/mod.rs
Outdated
/// | ||
/// # Returns | ||
/// | ||
/// Returns the non-zero combined size of all files, |
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.
/// Returns the non-zero combined size of all files, | |
/// Returns the non-zero combined size of all files in a directory, |
Awesome, makes sense. Thanks for the feedback! Lemme send another PR for you to see if it all looks good. |
You can update this PR (might require force-pushing to your branch) |
Hi @badboy, I am not sure why this is happening, but I see that one of the checks is failing. I'd appreciate it if you helped me figure out what the issue is. Thanks. |
That seems unrelated and you can ignore it. |
@badboy, I just fixed the issue. Thanks a lot for the guidance you provided on Bugzilla. Looking forward to some feedback.