BIND 10 #1704: log output mixed

[BIND 10 Development]

#1704: log output mixed
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120529
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  15
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Replying to [comment:53 jinmei]:
 > We need more detailed documentation for `InterprocessSync` and
 > `InterprocessSyncLocker`.  And, I'd generally suggest we complete doc
 > before review, because we often notice things like some hidden issue
 > or missing corner cases when we try to describe classes/methods (that
 > has been the case myself).  But for this particular ticket, we've
 > already spent too much time on it.  So maybe we should even defer it
 > to a separate ticket and close this one sooner.

 This has been added now.

 > '''interprocess_sync.h'''
 >
 > - Why did you make lock() variants protected and use `friend` to allow
 >  `InterprocessSyncLocker` to call them (from a quick look and check
 >  that seems to be the only purpose of the friend)?  In my understanding
 >  the more common convention is to just make such virtual methods public
 >  (no friend).  In some style it's recommended to make pure virtuals
 >  protected, but in that case the convention is (in my understanding) to
 >  provide corresponding non virtual methods to the base class, not
 >  relying on friend.  Besides, (I believe you know that but) `friend` is
 >  too powerful and should be considered a last resort weapon.  In
 >  general, if the friend class only needs to access friend's protected
 >  (not private) members, I believe it's better to achieve that goal with
 >  a less powerful tool than friend.

 It's done to somewhat force the use of InterprocessSyncLocker rather than
 InterprocessSync for locking (so that any locks are cleared up when the
 basic block is quit). This needs the friend as the lock methods in the
 InterprocessSync class are protected. I've described it in the class
 documentation now.

 > - I'd use 'const string&', and no, I don't think I modified it myself
 >   with my previous comment:
 > {{{#!cpp
 >     InterprocessSync(const std::string component_name) :
 >         component_name_(component_name), is_locked_(false)
 >     {}
 > }}}
 >   and make component_name_ const.  but this time I did it myself to
 >  save our time.  Same for `InterprocessSyncFile`, and I modified
 >  it, too.

 *nod* :)

 > '''interprocess_sync_file'''
 >
 > - I'd (explicitly) declare the `InterprocessSyncFile` destructor as
 >   virtual.  I thought it's also documented in the guideline.

 Done.

 > - If we use build dir, it's better to name the corresponding env
 >   variable "B10_FROM_BUILD".  "FROM_SOURCE" could be used to really
 >   specify a source directory.

 Done.

 > '''interprocess_sync_file_unittest'''
 >
 > - We might want to simplify timeval initialization:
 > {{{#!cpp
 >       struct timeval tv = { 5, 0 };
 > }}}
 >   but it may not be a big deal as we cannot make it const with that due
 >   to the select(2) interface.

 I was not sure from the posix definition of struct timeval if it can allow
 other members. This is why the members were set individually.

 > - I'd add comments why we use select().  Also explain why it's 5
 >   seconds, if only to say it's an arbitrary choice.

 Done.

 > '''logger.h'''
 >
 > - What happens if we pass NULL to `setInterprocessSync`?  (it may
 >   depend on that, but) I'd clarify this method is exception free,
 >   and that when non NULL the pointer must be delete-able and the
 >   ownership is transferred to `Logger` (so the caller doesn't have to
 >   / shouldn't delete it)

 A valid sync object always has to be present. I've made NULL do no change
 now.

 > - a minor point, but I'd place `setInterprocessSync` in logger.h
 >   somewhere separated from the log level/severity, etc related
 >   methods.  the current position seems a bit awkward.

 I've moved it to the end of the list now. Please see if it's ok.

 > '''logger_impl.cc'''
 >
 > - I believe you can now avoid including `<memory>`

 *nod*. :) Removed.

 > - LoggerImpl constructor: a minor deal, but you could initialize sync_
 >   in the initialization list:
 > {{{#!cpp
 > LoggerImpl::LoggerImpl(const string& name) :
 >     name_(expandLoggerName(name)),
 >     logger_(log4cplus::Logger::getInstance(name_)),
 >     sync_(new InterprocessSyncFile("logger"))
 > }}}

 Done.

 > - LoggerImpl::outputRaw: the locker is now an abstract object, not
 >   necessarily a file based:
 > {{{#!cpp
 > LoggerImpl::outputRaw(const Severity& severity, const string& message) {
 >     // Use a lock file for mutual exclusion from other processes to
 >     // avoid log messages getting interspersed
 > }}}

 Comment was updated.

 > '''log test'''
 >
 > - I suspect we don't have the chicken-egg problem here.
 > - I also suspect we don't need the B10_FROM_xxx trick here some (or
 >   perhaps most) of the cases because we replace the sync before the
 >   original one tries to open a file and not many tests actually try to
 >   dump logs.  btw the variable should better be named B10_FROM_BUILD
 >   where needed.

 Not most of the cases, but only one testcase where we actually test the
 logger for the lock. In other places where a mock object is not used,
 this is still needed. I've changed the use of the env variable to
 B10_FROM_BUILD everywhere now.

 > - technically, we should test the lock is acquired before the log
 >   output and released after that.  The Lock test doesn't actually
 >   confirm it.  But now that we've spent too much time on this ticket, if
 >   it's not easy to achieve, the current test is probably okay as a
 >   compromise.

 This will require something similar to the last design. It would need a
 wrapper
 around log4cplus so that logging calls are intercepted by a mock object
 *between*
 the lock/unlock. It would result in the same sort of ugliness as before.

 > '''bind10_src'''
 >
 > - remove_lock_files needs a test.

 Done. :)

-- 
Ticket URL: <http://bind10.isc.org/ticket/1704#comment:55>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development