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

Add time period offsets to calculation #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xombiemp
Copy link
Contributor

For each period we want to include the previous period's duration in
the calculation for when to delete snapshots.

For example, if we are saving 24 hours and 3 days, we don't want the
calculation for days to just be 3 days ago from the current time, we
want it to be 3 days ago from 24 hours ago.

To accomplish this, an offset has been added to the formula to include
the duration of the previous periods along with the current period's
duration.

For each period we want to include the previous period's duration in
the calculation for when to delete snapshots.

For example, if we are saving 24 hours and 3 days, we don't want the
calculation for days to just be 3 days ago from the current time, we
want it to be 3 days ago from 24 hours ago.

To accomplish this, an offset has been added to the formula to include
the duration of the previous periods along with the current period's
duration.
@xombiemp
Copy link
Contributor Author

Here are the changes you wanted done from my previous pull request. I also moved the deletes and periods definitions outside of the volumes loop so they aren't reinitialized on each volume. I can't see any problem with that... can you?

@xombiemp
Copy link
Contributor Author

Just wondering if there are other changes you would like to see before this request gets pulled?

@zwily
Copy link
Owner

zwily commented Oct 4, 2012

Conceptually this looks good, I'd just like to write a couple tests to verify the logic before I merge it. I think you're right about the intended behavior though.

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.

2 participants