Support rolling log files base on file size and time #811
Conversation
src/logging.cc
Outdated
| @@ -476,6 +494,9 @@ class LogFileObject : public base::Logger { | |||
| unsigned int rollover_attempt_; | |||
| int64 next_flush_time_; // cycle count at which to flush log | |||
| WallTime start_time_; | |||
| std::list<Filetime> file_list_; | |||
There was a problem hiding this comment.
What's the reason for choosing a std::list here? Looks like you want a std::priority_queue here since you are constantly sorting the container elements.
Also the member variable name should indicate what kind of times are stored.
There was a problem hiding this comment.
Thanks for review, it is because of need to access the last element @
Line 1252 in 0f7ffec
There was a problem hiding this comment.
That's not a good reason for choosing std::list. Given you are continually sorting, std::priority_queue is probably a better choice.
There was a problem hiding this comment.
Maybe std::set is a better choice? It is ordered and have access to first and last element.
There was a problem hiding this comment.
No, std::set is only useful if you lookup specific (unique) elements which you don't. The container also doesn't really give direct access to the top element. This is what std::priority_queue is for.
src/logging.cc
Outdated
| @@ -1148,13 +1221,50 @@ void LogFileObject::Write(bool force_flush, | |||
| if (base_filename_selected_ && base_filename_.empty()) { | |||
| return; | |||
| } | |||
|
|
|||
| if (file_length_ >> 20U >= MaxLogSize() || PidHasChanged()) { | |||
| struct ::tm tm_time; | |||
There was a problem hiding this comment.
The following block does quite some work. Consider moving it into a separate method and add some comments describing the overall purpose and the non-obvious logic.
src/logging.cc
Outdated
| @@ -1164,7 +1274,15 @@ void LogFileObject::Write(bool force_flush, | |||
| if (++rollover_attempt_ != kRolloverAttemptFrequency) return; | |||
| rollover_attempt_ = 0; | |||
|
|
|||
| struct ::tm tm_time; | |||
| if (!inited_) { | |||
There was a problem hiding this comment.
Some comments of what happens here would be nice.
yangzhg
force-pushed
the
support_rolling
branch
from
b3c3b4d
to
caa420a
Compare
|
@sergiud I have edited according to your comments, please review again |
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
==========================================
- Coverage 73.19% 72.87% -0.32%
==========================================
Files 17 17
Lines 3279 3366 +87
==========================================
+ Hits 2400 2453 +53
- Misses 879 913 +34
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Thanks! Unfortunately, Windows unit tests are failing now. Could you please take a look at these first? |
Ok. I will find a Windows device to test it in this weekend |
I have tested it on Windows vs 2022 and not failed now, would you please trigger the build again? |
Add two flags max_logfile_num and log_rolling_policy, support rolling log file by day or by hour, and use max_logfile_num to control the max history log file num after rolling.
sergiud
force-pushed
the
support_rolling
branch
from
9f2a72b
to
c09d50a
Compare
| int64 next_flush_time_{0}; // cycle count at which to flush log | ||
| WallTime start_time_; | ||
| std::multiset<Filetime> file_list_; |
There was a problem hiding this comment.
I'm not sure why a std::multiset is needed here. Can a file be associated with multiple timestamps? I don't see how this is possible. Either way, a multiset would be rather memory inefficient here, since the same file name would be potentially stored multiple times.
Please drop the Filetime structure and use an associative container. I guess a std::map is sufficient here.
|
I just pull the branch code test roll file function, when restart test demo many times the file count will over FLAGS_max_logfile_num. Is this a problem ? |
|
@IceLemon99 I'll close the PR since it's heavily diverged from |
This pr implements #809
Add two flags max_logfile_num and log_rolling_policy, support rolling
log file by day or by hour, and use max_logfile_num to control the max
history log file num after rolling.