Boost logo

Boost :

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
> > 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

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.

Boost list run by bdawes at, gregod at, cpdaniel at, john at