forked from nsqio/go-diskqueue
-
Notifications
You must be signed in to change notification settings - Fork 4
Add retention time to keep data during the last X hours. #9
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
Open
CatherineF-dev
wants to merge
1
commit into
kev1n80:RetentionTime
Choose a base branch
from
CatherineF-dev:retentionTime
base: RetentionTime
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 outdated files are deleted when diskqueue reaches size limit. in other words, they stay and will still be read as long as total size is within limit. In this case there seems not much improvement on setting total size limit alone.
I think it makes more sense (if a retention time is set) that expired files would not even be read and be deleted as soon as they expire. so that total size limit and the time limit both serves to reduce space usage and which ever comes first would take effect.
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.
+1 I was wondering if you could check retention size:
time.NewTicker()
similar to syncTicker. The ticker will start if it is null and a log file exists, for the duration ofretentionTime - (currentTime - oldestFileCreateTime)
. When the ticker goes off it will iterate starting from the oldest file and delete outdated files.readOne()
This way. as Leon mentioned above, the retention size and retention time both serve to reduce space usage and whichever occurs first would take effect.
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.
If we want to use this now and go with the current approach I would suggest either:
retentionTime
, because it's only enforcing retention time in the size limit exceeding case;retentionTime
name but add more comments/descriptions and a TODO as a reminder to implement the other half (enforcing retention time under the size limit).