-
Notifications
You must be signed in to change notification settings - Fork 453
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
Make snapshot process snapshot all unflushed blocks #1017
Conversation
richardartoul
commented
Oct 4, 2018
•
edited
Loading
edited
- - Modify snapshot behavior to snapshot all unflushed blocks within the retention period
- - Update integration tests to capture new behavior
Codecov Report
@@ Coverage Diff @@
## master #1017 +/- ##
========================================
- Coverage 71.1% 71.1% -0.1%
========================================
Files 732 732
Lines 61493 61520 +27
========================================
+ Hits 43749 43763 +14
- Misses 14922 14937 +15
+ Partials 2822 2820 -2
Continue to review full report at Codecov.
|
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.
LGTM with a comment, but might want to get Prateek/Rob to look over this too.
src/dbnode/storage/util.go
Outdated
@@ -49,7 +49,7 @@ func timesInRange(startInclusive, endInclusive time.Time, windowSize time.Durati | |||
return nil | |||
} | |||
times := make([]time.Time, 0, ni) | |||
for t := endInclusive; !t.Before(startInclusive); t = t.Add(-windowSize) { |
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.
Were there assumptions elsewhere in the code that relies on the time ranges always including the end time? Also fix the comment for this function.
@richardartoul talked offline: maybe add a stat for the number of blocks being snapshot |
c7b8972
to
ee4f425
Compare
snapshotFiles, err := fs.SnapshotFiles( | ||
filePathPrefix, namespace, shard) | ||
if err != nil { | ||
panic(err) |
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.
don't panic here, propagate error
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.
meh just saw its only for integration tests, w/e. feel free to leave as is.
candidateTimes := timesInRange(earliest, latest, blockSize) | ||
return filterTimes(candidateTimes, func(t time.Time) bool { | ||
// Snapshot anything that is unflushed. | ||
return ns.NeedsFlush(t, t) |
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.
do you need to check snapshot enabled here
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.
LGTM