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

Age rotation for old data #1103

Merged
merged 9 commits into from Oct 28, 2020
Merged

Age rotation for old data #1103

merged 9 commits into from Oct 28, 2020

Conversation

lava
Copy link
Member

@lava lava commented Oct 20, 2020

Review instructions: Each commit can be reviewed individually, although 90% of the changes are in the last commit anyways.

@lava lava force-pushed the story/ch19925/age-rotation branch 5 times, most recently from aee289c to 283f03e Compare October 20, 2020 13:08
@mavam mavam changed the title wip: Age rotation for old data Age rotation for old data Oct 20, 2020
@mavam mavam added the feature New functionality label Oct 20, 2020
@lava lava force-pushed the story/ch19925/age-rotation branch 3 times, most recently from c6c732c to 2b83441 Compare October 22, 2020 18:31
@lava lava marked this pull request as ready for review October 22, 2020 18:31
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Great PR! I only have smaller things to discuss.

Since we're doing out-of-band testing with this PR, I have only looked at the code from a mechanical point of view.

libvast/vast/concept/parseable/vast/si.hpp Outdated Show resolved Hide resolved
libvast/src/directory.cpp Outdated Show resolved Hide resolved
libvast/src/directory.cpp Show resolved Hide resolved
libvast/vast/directory.hpp Outdated Show resolved Hide resolved
integration/default_set.yaml Outdated Show resolved Hide resolved
libvast/src/system/index.cpp Outdated Show resolved Hide resolved
libvast/src/system/spawn_disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/spawn_disk_monitor.cpp Show resolved Hide resolved
libvast/src/system/spawn_disk_monitor.cpp Show resolved Hide resolved
libvast/src/system/spawn_node.cpp Outdated Show resolved Hide resolved
@lava lava force-pushed the story/ch19925/age-rotation branch 2 times, most recently from f7866f3 to 7a9bbb1 Compare October 26, 2020 20:39
@dominiklohmann dominiklohmann mentioned this pull request Oct 27, 2020
9 tasks
@lava lava force-pushed the story/ch19925/age-rotation branch from 7a9bbb1 to d7b562c Compare October 27, 2020 10:25
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Some comments, mostly on log message wording.

This needs two changelog entries: A change for the archive options, and an experimental feature for the disk monitor. Please reflect all new and changed options in the example configuration.

The integration test should be disabled on macOS as discussed via Slack. .DS_Store files don't play well with how we're currently calculating the directory size and how the test is currently implemented.

libvast/src/system/application.cpp Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/spawn_disk_monitor.cpp Show resolved Hide resolved
Add a method to compute recursive directory size.

Also make some members of `vast::directory` const in order
to support usage of the class in a const context.
@lava lava force-pushed the story/ch19925/age-rotation branch 2 times, most recently from e1ea50b to c68d6df Compare October 27, 2020 12:09
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Looks good from a 30k-foot view now. Just some cosmetic changes.

I didn't check the application logic or test the commit, it would be great if somebody else from @tenzir/backend can take care of that.

libvast/src/system/spawn_disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/src/system/spawn_disk_monitor.cpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/si.hpp Show resolved Hide resolved
libvast/vast/directory.hpp Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dominiklohmann
Copy link
Member

@mavam
I didn't check the application logic or test the commit, it would be great if somebody else from tenzir/backend can take care of that.

I've verified this to work locally on macOS. It's fine for an experimental feature, I think.

@lava lava force-pushed the story/ch19925/age-rotation branch from c68d6df to 2ef5383 Compare October 27, 2020 13:28
@lava lava requested a review from mavam October 27, 2020 13:32
integration/default_set.yaml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
libvast/src/directory.cpp Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Outdated Show resolved Hide resolved
Fix a double-free bug when copying a vast::directory,
that was caused by a Rule-of-Three violation; i.e. when
copying a directory the contained `DIR*` was simply copied
and then closed multiple time in the destructor.
@lava lava force-pushed the story/ch19925/age-rotation branch from 2ef5383 to af144d0 Compare October 27, 2020 15:34
Add a new 'disk_monitor' actor that periodically
monitors disk usage of the vast database directory
and triggers removal of old partitions.
@lava lava merged commit 142e012 into master Oct 28, 2020
@lava lava deleted the story/ch19925/age-rotation branch October 28, 2020 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
3 participants