Re: [boost] [log] Patch: rotating file collector

Subject: Re: [boost] [log] Patch: rotating file collector
From: Alexander Arhipenko (arhipjan_at_[hidden])
Date: 2013-05-06 08:09:21

Hi Andrey,

Again, thanks for you comments.

I've fixed several items mentioned in code review.
To be precise: 1, 3, 4.
1: factory function name, make_rollover_collector was chosen.
3:
> 3. In rotating_file_collector::scan_for_files, invalid file name pattern
> should also be handled with setup_error.
>
4:
> 4. I think, it should be possible to avoid the do_rollover method.
> file_collector_base::store_file should be moved to file_collector, and there
> should be a separate rotating_file_collector::store_file method as well. Any
> common subroutines (like checking available free space, erasing old files,
> etc.) can be extracted as distinct functions into file_collector_base and
> called from store_file methods of the derivatives.
>

Also, patch was rebased against boost trunk.

Regarding points 5 and 6: after analyzing this case,
I've dropped support for datetime pattern in rollover collector.
Please, find some explanations below.
First, using last write time for the file name pattern was not adopted
unambiguously by community. Also, there is a counter example in existing
component - python TimedRotatingFileHandler, that uses creation timestamp.
Second, if user wants to use file creation timestamp in rotated file name,
she can use existing make_collector function for that purpose.
You've also mentioned an option of using datetime placeholder in file sink:

> ... Maybe it's better to avoid
> dealing with date/time placeholders in the collector at all and only work with
> file counters. But scan_for_files should be changed so that it is possible to
> set a pattern with date/time placeholders in the sink backend.

I liked that option, but existing implementation should be enhanced to
support this: currently during rollover phase files are simply renamed.
I.e., fileN -> fileN+1, fileN-1 -> fileN, ..., file -> file1.
Thus, datetime placeholder will be invalid after rollover -
it will point to the wrong date.
If you think this option is very nice to have - please tell.

There are still 2 points we need to reach an agreement on:

a. > > 2. The new collector should also be kept in the collectors repository.

b. > * there is no need to rotate_file during text_file_backend destruction.

In regards to a. - collectors repository.
I agree that only one file_collector should serve one target directory.
Regarding rollover collectors - they are different beasts.
They do not serve the whole directory, but only file series in that directory.
So, I propose 2 alternatives:
a) use target directory and file name pattern as collector "identity"
or
b) do not put rollover collector into repository

Now quoting some of your arguments against:

> b. If Boost.Log is managing the storage, you set up the collector and mostly
> rely on the fact that noone else is messing around with the storage. This
> means that we don't try to detect sudden file deletions or additions to the
> storage, although should that happen the collector should handle the situation
> gracefully. This means that if there are two services (foo and bar) running,
> they should be collecting their log files to separate directories. This is
> fairly common in Linux, at least, since many services would just create
> directories within /var/log and put their logs there. If this is not
> acceptable, then go for case (a).
> [snip]
> ... The problem of having different
> collectors targeting the same directory can be solved by simply prohibiting
> such configuration, because the desired behavior is not clear in this case.

Agree that Boost.Log should not detect any "external" file deletions or
additions. However, I don't see any problems of having 2 _rollover_
collectors targeting one directory.
Each of them will monitor own file series and manipulates (add, remove)
only on that series.
The only 1 point of "race condition" we can imagine -
that is minimum free space requirements.
However, this is still not a problem: even if both collectors perform
rotation at one point in time and minimum free space requirement is not met,
each collector will just remove its file(s),
freeing a little bit more of free space.

Also, some words regarding Linux example.
On my Ubuntu 12.04 there are both files that reside in separate directories:
/var/log/samba, /var/log/mongodb, /var/log/nginx
as well as in root log directory:
/var/log/syslog.log*, /var/log/dpkg.log*, /var/log/auth.log*.
That means that having files from different services in one directory
is existing practice. We need to give user an ability to cover this cases
in Boost.Log library.

In regards to b. - rotate_file during text_file_backend destruction.
You've provided a good argument that I can't argue against:
>[snip]
> ... Imagine there are timestamps in the file name,
> so the next time you start your application the file names will
> be different and the old files will be left unmanaged. So the files are
> collected when the sink is destroyed and then managed by the collector after
> the application restarts and runs scan_for_files.

However, I still think that having active log file with persistent name
will be a good benefit for many cases
(one of the examples - /var/log/*.log* files mentioned above).
After some thinking, I've came to conclusion that when
* file sink does not have placeholders (i.e., log file name is static),
* active log resides in collection directory (this point is not mandatory)
- then we can safely do not rotate such file during sink destruction.
So, I propose to implement the behavior described above, i.e.:
log file is not rotated during sink destruction when it's filename is static.

Regards