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

Comment (by jinmei):

 Replying to [comment:52 muks]:

 The latest version seems closer to merge.  I still have some comments
 (below), but they are not design level ones and are generally minor or
 trivial.

 > > - I'd rather specify the full path than relying on the B10_FROM_ trick
 > >   with setting in main() globally:
 > > {{{#!cpp
 > >   InterprocessSync* sync = new InterprocessSyncFile("test");
 > > }}}
 > >   Setting this environment variable could affect other tests, and the
 > >   full path is in fact closer to what would be used in the actual
 > >   deployment.  Note that if we do this we can/should remove setenv
 > >   from main().
 >
 > I don't follow completely here. The constructor accepts just the
 component name, and builds a path according to the runtime/test
 environment. It has to be able to accept different paths as the install
 prefix may not be writable or even exist. I would not prefer allowing to
 specify the full pathname to the lock file by the caller, as the lockfile
 locations can be managed by us, or may not even
 > be an actual file.

 Ah, okay, I was confused a bit; we cannot really specify the directory
 for the file.  I have some related comment on the revised version.
 See the "B10_FROM_xxx trick" thing on the log test comments.

 Comments on the revised version:

 '''general'''

 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.

 I made a few more editorial fixes.

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

 '''interprocess_sync_file'''

 - I'd (explicitly) declare the `InterprocessSyncFile` destructor as
   virtual.  I thought it's also documented in the guideline.
 - 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.

 '''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'd add comments why we use select().  Also explain why it's 5
   seconds, if only to say it's an arbitrary choice.

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

 '''logger_impl.cc'''

 - I believe you can now avoid including `<memory>`

 - 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"))
 }}}
 - 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
 }}}

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

 '''bind10_src'''

 - remove_lock_files needs a test.

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