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

On Tuesday 23 April 2013 16:18:44 you wrote:

On Tue, Apr 23, 2013 at 3:58 PM, Alexander Arhipenko 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.