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

Subject: Re: [boost] [log] Patch: rotating file collector
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2013-04-23 16:27:43

On Tuesday 23 April 2013 16:18:44 you wrote:
> On Tue, Apr 23, 2013 at 3:58 PM, Alexander Arhipenko 
<arhipjan_at_[hidden]>wrote:
> > I've added implementation of rotating file collector -
> > the patch against boost log trunk attached.
> > This is not the final version, I'm going at least to finish most of the
> > TODOs.
> > 
> > However, it would be nice to receive feedback and questions from
> > library author as well as from people interested in this feature.
> > 
> > Andrey, could you please find several minutes to review the attached
> > patch?
> 
> Thanks for the patch. I didn't look deeply into it yet but from the first
> glance it looks like you changed the existing file collector rather than
> introduce an alternative one. Am I right? If so, could this be made as a
> pure addition rather than modification? I would prefer to keep the current
> behavior also available for the users.

I took a closer look now and my first impression was wrong - the original 
behavior is still accessible, this is good. However, I have several notes:

1. I would still prefer the interface to be distinct for different types of 
collectors, and not based on the file name pattern presence. These are 
distinct objects with different behaviour and it should be apparent from the 
collector construction what behavior it will implement.

I'm prepared to rename the current file::make_collector method to better 
describe the current behavior (I'm open to suggestions on the new name). The 
new collector should be created with a different function, also appropriately 
named (file::make_stacking_collector?). It's also probably a good idea to 
extract collectors and the base file::collector interface to separate headers.

2. The new collector should also be kept in the collectors repository. This 
way it is possible to create multiple sinks collecting files in the common 
target directory, without different collectors interferring with each other. 
Basically, you'll have to move file_collector_hook and 
enable_shared_from_this< file_collector > base classes to file_collector_base, 
as well as m_pRepository.

This introduces an additional possible issue if the user tries to create 
different sinks with different collectors targeting the same directory. This 
case should be detected (e.g. by checking the type of the collector in 
file_collector_repository::get_collector) and handle it with an exception 
(log::setup_error).

3. In rotating_file_collector::scan_for_files, invalid file name pattern 
should also be handled with setup_error.

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.

5. You don't seem to rename the collected files when the file pattern contains 
date/time (see date_rollover_policy). Is that right? Obviously, you will only 
have the date and time point of the rotation and not the file creation.

6. Actually, the date/time support for the new collector seems quite broken. 
The current behavior, when the file is originally named by the sink, ensures 
that the date/time in the file name corresponds to the first records in the 
file. If the date/time is added by the collector, the timestamp will 
correspond to the latest written records, which it counterintuitive. The check 
in rotating_file_collector::scan_for_files prevents users from setting 
date/time in the sink, so basically, it will only be usable if there is only a 
file counter placeholder in the file name pattern.

I'm not sure what is the best way to solve this. 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. In that case I 
think it's reasonable to remove the file name pattern from the file collector 
construction and simply always add file counters right before the extension. 
That way the file name pattern is only set in the sink backend, which 
simplifies the interface a lot.

7. You should probably rebase your patch to the Boost trunk, as the library 
has been merged recently. For now it's pretty close to the SourceForge SVN, so 
you should not have any problems. I'm going to work in Boost SVN now, so the 
SourceForge SVN will get outdated eventually.

> Also, I would like to discuss following question with you :
> > rotate_file invocation in ~text_file_backend -
> > I would prefer to have this call during text_file_backend construction
> > phase.
> 
> I'll have to take a closer look later to answer that but it seems odd to
> have to rotate something when nothing have been written yet. The
> rotate_file call is intended to place the written file to the storage
> governed by the collector, and at construction this file hasn't been
> created yet. Could you describe why you need this?

This question remains actual.