BIND 10 #764: Conversion of BIND10 Python library to use new logging interface

[BIND 10 Development]

#764: Conversion of BIND10 Python library to use new logging interface
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110712
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  7.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''general'''

 I made some doxygen related fixes, which I believe are trivial.

 '''bindcmd.py'''
 - What's the purpose of the change?  It looks irrelevant to this
   branch.

 '''log/compiler/message.cc'''

 - In the following diff, the use policy of backslash is inconsistent:
 {{{
 -/// <tt>message [-v | -h | \<message-file\>]</tt>
 +/// <tt>message [-v | -h | -p | -d <dir> | \<message-file\>]</tt>
 }}}
   I don't understand why the backslash was used in the original code
   (it seems to be unnecessary), but it seemingly tries to escape < and
   >.  If so, shouldn't it also be used for <dir>?  But again, I'd
   rather remove the backslash for <message-file>
 - on a related note, I suspect the backslash in the following line
   should also be removed (in fact, in the doxygen output in my
   environment these backslash'es remain.
 {{{
  /// \-v causes it to print the version number and exit. \-h prints a help
 }}}
 - Also, you may want to update the following sentence due to the
   introduction of -d:
 {{{
  /// It reads the message file and writes out two files of the same name
 in the
  /// default directory but with extensions of .h and .cc.
 }}}
   (Although I don't know what the "default directory" means,
   considering the fact that this term was used before the introduction
   of -d)
 - we shouldn't rely on direct conversion from pointer to bool.  same for
   writeHeaderFile (directly fixed).
 {{{
 +    if (output_directory) {
 +        python_file.setDirectory(output_directory);
 +    }
 }}}
 - I made some trivial style fixes with the fix for the previous one.

 '''python/isc/config/Makefile.am'''
 - (Though matter of taste) these lines look too long to me:
 {{{
 pyexec_DATA = cfgmgr_messages.py
 $(top_builddir)/src/lib/python/config_messages.py
 ...
         $(top_builddir)/src/lib/log/compiler/message -p -d
 $(top_builddir)/src/lib/python
 $(top_srcdir)/src/lib/python/isc/config/config_messages.mes
 }}}
   I'd fold them (with escaping when necessary).
 - This is not specific to this branch, but I've noticed one
   controversial point.  Due to this the message py will be installed
   under pyexecdir (such as /usr/local/lib/python3.1/site-packages/):
 {{{
 pyexec_DATA = cfgmgr_messages.py
 $(top_builddir)/src/lib/python/config_messages.py
 }}}
   I don't think it good.  First, to be in such a generic package
   directory, the file names are too generic (what kind of "message" is
   it?, which configuration is it for?).  It would also look awkward in
   the caller (such as ccsession.py) side: most of BIND 10 modules are
   imported with a prefix of "isc", the log messages are missing it.
   IMO, they should go under (conceptually)
   something like $(pyexecdir)/isc/log/messages or
   $(pyexecdir)/isc/log/<module>.  Coincidentally, while I work on
   #983, I encountered the same issue (for a different reason), and I
   found it difficult to address within the automake framework.  I may
   be able to resolve it in #983, then we can use the same technique
   for log messages.  In any case, this would be beyond the scope of
   this ticket, and I'm okay with deferring it to a separate ticket.

 '''python/isc/notify/Makefile.am'''
 - The same comments for config/Makefile.am apply here.

 '''notify_out.py (and the corresponding message file)'''
 - shouldn't the call to _handle_notify_reply() from
   _zone_notify_handler have the added fourth parameter?  (This would
   also mean there's a missing test case)
 - NOTIFY_OUT_IMPORT_DNS isn't defined:
 {{{
     logger.error(NOTIFY_OUT_IMPORT_DNS, str(e))
 }}}
   (this made me ask the usual comment: please perform consistency
   checks on the added log IDs and messages throughout the branch)
 - Also, I don't remember the conclusion (if any) of the previous
   discussion, but I thought we previously added a suffix to log IDs
   that indicates a type of message, e.g. _ERROR.
 - And, I found a more fundamental problem here (not specific to this
   branch though): when this log is triggered, it's quite unlikely that
   the logging has been initialized.  I suspect there are other such
   cases.
 - NOTIFY_OUT_TIMEOUT_RETRY: The detailed description doesn't seem to
   be very accurate, because the retry may fail due to the quota
   immediately below this point.  I'd revise it to, e.g.:
 {{{
 The notify message to the given address (noted as address#port) has
 timed out, and the message will be resent until the max retry limit
 is reached.
 }}}
   For the same reason, we might simply name the ID "..._TIMEOUT", not
   _RETRY.
 - NOTIFY_OUT_RETRY_EXCEEDED: I'd use a higher level than info (if any)
   for this event because it should be unusual.
 - NOTIFY_OUT_RETRY_EXCEEDED: maybe it's helpful to log the max number,
   too.
 - NOTIFY_OUT_SOCKET_ERROR: "network *connection* error" doesn't sound
   very accurate for UDP.  Maybe just say "network error"?
   If possible (and if any), I'd describe a typical case where that
   happens.  Maybe one common case is lack of routes.
 - NOTIFY_OUT_INVALID_ADDRESS: not actually for this ticket, but could
   that normally happen?  If so, the code should be revised so that it
   will reject invalid form of address at a higher level (e.g. when
   parsing config/command update).  If it can only in such special
   cases as tests, I'd note that this shouldn't normally happen in the
   description.
 - Why is this commented out rather than just removed?
 {{{
 +            #errstr = 'notify reply error: '
 }}}
 - _handle_notify_reply(): the pydoc for the function mentions error
    message.  you may want to update that part.
 - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd use lower level than error, because
   it's not our error.  Same for other BAD_ cases.
 - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd describe what will happen for the
   notify request in this case.  Specifically, whether it will be
   resent.  Same for other BAD_ cases.
 - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd say "message" instead of "packet".
   Same for other BAD_ cases.
 - hmm, after seeing all bad reply messages, I wonder whether we really
   need this level of granularity for such quit rare events.  But it
   wouldn't be harmful either (except for making the source code
   longer/larger marginally), so I don't insist (e.g.) these be unified.
 - NOTIFY_OUT_REPLY_UNCAUGHT_EXCEPTION: "uncaught exception" could be
   vague about the context.  I'd clarify where that happens.
 - NOTIFY_OUT_SOCKET_RECV_ERROR: I'd avoid using "connection".  If
   possible (and if any), I'd describe a typical case where that
   happens.
 - _get_notify_reply(): not specific to this branch, but what if
   tgt_addr != addr?

 '''util/filename.h,cc'''
 - Maybe it's acceptable if it's only used for tests and the log
   message complier (which is the case right now), but setDirectory()
   doesn't provide the strong exception guarantee; if, e.g., creating
   a new full name results in an exception, the Filename object would
   have inconsistent state.  BTW, it doesn't seem to be very difficult
   to provide the strong guarantee if you use string::swap().
 - Some behavior of setDirectory() does not seem intuitive or trivial
   to me: it accepts an empty string and treats it as "/"; If Filename
   has been constructed as an absolute path, setDirectory() will
   replace the "closest enclosure" directory.  Personally, I'd reject
   an empty string.  Then the "find, then append if necessary" logic
   will be much simpler.  But in any case, I don't necessarily object
   to the current behavior if you like it.  If you keep the behavior,
   however, please explicitly describe that in the doxygen doc.
 - what if new_directory has a redundant trailing '/'? e.g.
 /just/some/dir//?

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