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
File size utilities #764
File size utilities #764
Conversation
the size values from hdf5 1.10.7 were used
Codecov Report
@@ Coverage Diff @@
## master #764 +/- ##
==========================================
+ Coverage 83.62% 83.70% +0.07%
==========================================
Files 66 66
Lines 4496 4542 +46
==========================================
+ Hits 3760 3802 +42
- Misses 736 740 +4
|
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.
Very nice, thank you! There's a few things I'd suggest changing. If you can take care of them that would be appreciated, if not we can do it.
@@ -110,6 +110,12 @@ class File: public Object, public NodeTraits<File>, public AnnotateTraits<File> | |||
return details::get_plist<FileAccessProps>(*this, H5Fget_access_plist); | |||
} | |||
|
|||
/// \brief Get the disk size of this file in bytes | |||
size_t getDiskSize() const; |
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 a wrapper for the function H5Fget_filesize
. Could we use getFileSize
instead?
|
||
|
||
{ | ||
File file(file_name_untracked, File::ReadWrite | File::Create | File::Truncate); |
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.
FYI this can be reduced down to:
File file(file_name_untracked, File::Truncate);
It's consistent with the rest of the file so there's no reason to change it.
@@ -426,6 +426,56 @@ TEST_CASE("Test groups and datasets") { | |||
} | |||
} | |||
|
|||
#if H5_VERSION_GE(1, 10, 1) | |||
TEST_CASE("FileSizeAndUnusedSpace") { |
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.
Please split the test in two. One for the untracked file and one for the tracked variant.
@@ -426,6 +426,56 @@ TEST_CASE("Test groups and datasets") { | |||
} | |||
} | |||
|
|||
#if H5_VERSION_GE(1, 10, 1) |
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.
The reason it's needed it so set the file space strategy. The newly wrapped functions were introduced in HDF5 1.6.x and don't need guards.
File file(file_name_tracked, File::ReadWrite); | ||
CHECK(file.getDiskSize() == tracked_file_size); | ||
CHECK(file.getUnusedSpace() == tracked_unused_space); | ||
} |
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.
As you already said, these will fail. Since we're not testing if HDF5 is correct, it might be enough to check that:
- the file size is
>0
. - the free space is
== 0
(when applicable). - the free space is
> 0
and< file_size
(when applicable).
Thank you for the PR, I'll merge it into a feature branch and apply the changes I mentioned. |
Description
Two methods are added to the File interface : one to get the file disk size through H5Fget_filesize and the currently tracked unused space through H5Fget_freespace.
These can be useful when using a persistant unused space tracking strategy to decide when to trigger repacks after data multiple addition and deletion of data.
Unit tests have been written, however it may be irrelevant since the disk size may vary depending on the hdf5 library version.
How to test this?
cmake .. make -j8 make test
Test System