-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
compactor: Make periodic compactor runs every hour #7875
Conversation
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.
OK in general
compactor/compactor.go
Outdated
@@ -41,6 +42,8 @@ type RevGetter interface { | |||
Rev() int64 | |||
} | |||
|
|||
// Periodic compacts the log by purging revisions older than | |||
// the configured retention time. Compaction happenes every hour. |
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.
s/happenes every hour/happens hourly
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.
Thanks! Done.
compactor/compactor.go
Outdated
return -1 | ||
return -1, t.revs | ||
} | ||
if i >= len(t.revs) { |
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.
would/should this ever evaluate as true?
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.
With the current code, it's evaluated only when h
is 0
.
It was for just in case, so I removed it.
compactor/compactor_test.go
Outdated
} | ||
|
||
// compaction doesn't happen til 2 hours elapses | ||
if i+1 >= retentionHours { |
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.
save an indent level?
if i+1 < retentionHours {
continue
}
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.
Done.
compactor/compactor.go
Outdated
@@ -41,6 +42,8 @@ type RevGetter interface { | |||
Rev() int64 | |||
} | |||
|
|||
// Periodic compacts the log by purging revisions older than | |||
// the configured retention time. Compaction happenes hourly. |
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.
s/happenes/happens
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.
Oops, sorry. Fixed.
/cc @xiang90 |
lgtm, thanks! @xiang90 may have some additional thoughts on this since he wrote the original, but this seems OK to me. Compaction is optimized some so the performance impact from the additional compacts should be negligible. The description of |
LGTM. I am aware of this potential issue. Not sure if I should do it at that time. @yudai Thanks for improving this. |
Codecov Report
@@ Coverage Diff @@
## master #7875 +/- ##
=========================================
Coverage ? 75.93%
=========================================
Files ? 332
Lines ? 26205
Branches ? 0
=========================================
Hits ? 19900
Misses ? 4891
Partials ? 1414
Continue to review full report at Codecov.
|
Closes #7868.