-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 map between severity and overdue minutes #1144
base: master
Are you sure you want to change the base?
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.
Thanks for the PR and apologies for the late reply.
The changes are going into the right direction. Please add some unit tests. For this, you can extend the existing log cleaner tests.
src/logging.cc
Outdated
@@ -1286,7 +1299,14 @@ LogCleaner::LogCleaner() = default; | |||
|
|||
void LogCleaner::Enable(const std::chrono::minutes& overdue) { | |||
enabled_ = true; | |||
overdue_ = overdue; | |||
for (int i = GLOG_INFO; i < NUM_SEVERITIES; i++) { | |||
overdue_per_severity_[static_cast<LogSeverity>(i)] = overdue; |
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.
Fill the map only with the severity set by the user.
cb1b4aa
to
55e6f47
Compare
Hey @sergiud , I have made some improvements, I switched from a map to a vector for better lookup speed and cleaner implementation. I also addressed all the issues you pointed out. PTAL! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1144 +/- ##
==========================================
- Coverage 62.22% 61.95% -0.27%
==========================================
Files 19 19
Lines 2324 2363 +39
Branches 845 856 +11
==========================================
+ Hits 1446 1464 +18
- Misses 596 616 +20
- Partials 282 283 +1
|
f0e5b47
to
314903e
Compare
Hey @sergiud, I initially made the wrong assumptions, I thought log files must have severity in their names, but that’s not the case. I have added methods to enable, disable logging for both types of files (with and without severity in their names) and exported those functions. I also added a test file that checks for deferred cleanup logic, but I haven’t integrated it with CMake yet. |
I have added an
unordered_map
that can store a severity and it's corresponding overdue time to theLogCleaner
object. The modifications should be backwards compatible. If this idea is correct, we can preform some optimizations, like keeping a cache of filepaths whose severities have been computed.Fixes: #882