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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:8 jinmei]:
 > '''bindcmd.py'''
 > - What's the purpose of the change?  It looks irrelevant to this
 >   branch.
 >

 there was a print() statement in the ccsession library call made there,
 and bindcmd is the 'user-facing' part (as an integral part of the binctl
 cli), while ccsession may not be. So i essentially moved the print from
 ccsession.py to bindcmd.py

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

 removed the backslashes


 > - Also, you may want to update the following sentence due to the
 >   introduction of -d:

 updated (made it 'current working directory (unless -d is used)')


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

 ok, done

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

 So far this was a deliberate choice as a tradeoff for source vs. build
 directory trees and the way python resolves library components. If you can
 come up with a better solution I'm certainly fine with that. Otherwise we
 can consider changing the filenames to make it more clear.

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

 ack

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

 ack, added test and fixed call

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

 yeah, i actually did run through the messages, but this one somehow
 slipped by

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

 yes this is something i have been starting to worry about also, not only
 that it hasn't been initialized (which, if so, should be easy to fix), but
 also that even if initialized, it'll also come before -v is read, not to
 mention general configuration. Since this is an error, it doesn't matter
 on severity-level right now, but it still something we need to think
 about.


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

 ok


 >   For the same reason, we might simply name the ID "..._TIMEOUT", not
 >   _RETRY.

 ok

 > - NOTIFY_OUT_RETRY_EXCEEDED: I'd use a higher level than info (if any)
 >   for this event because it should be unusual.

 error?

 > - NOTIFY_OUT_RETRY_EXCEEDED: maybe it's helpful to log the max number,
 >   too.

 ok

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

 removed connection, added 'The address might be unreachable'.

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

 technically it can always happen; the command comes from any module, ours
 or external, and yes, they should verify what they send, but we still need
 to catch it if they don't.
 Added
 "or a different BIND 10 module has forgotten to validate its data before
 sending this module a notify command. As such, this should normally
 not happen, and points to an oversight in a different module.
 "
 But i'm not really a fan of 'this should happen', actually :p


 > - Why is this commented out rather than just removed?
 > {{{
 > +            #errstr = 'notify reply error: '
 > }}}

 removed


 > - _handle_notify_reply(): the pydoc for the function mentions error
 >    message.  you may want to update that part.

 just removed that line


 > - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd use lower level than error, because
 >   it's not our error.  Same for other BAD_ cases.

 warn ok?

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

 added that we did get a response, so we don't resend the notify (which i
 think is the reasoning behind the behaviour)

 > - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd say "message" instead of "packet".
 >   Same for other BAD_ cases.

 ok

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

 ok leaving them as they are for now


 > - NOTIFY_OUT_REPLY_UNCAUGHT_EXCEPTION: "uncaught exception" could be
 >   vague about the context.  I'd clarify where that happens.

 added a bit about that it's either in the parser or when trying to extract
 data from the parsed packet


 > - NOTIFY_OUT_SOCKET_RECV_ERROR: I'd avoid using "connection".  If
 >   possible (and if any), I'd describe a typical case where that
 >   happens.

 removed connection. I suspect that if this error occurs, the actual error
 message will be more helpful than trying to come up with likely scenarios
 here.


 > - _get_notify_reply(): not specific to this branch, but what if
 >   tgt_addr != addr?
 >

 hmm, yeah, i'd guess we should warn but continue (since this does tend to
 occur once in a while, though it's fixed in most server), i'm not entirely
 sure whether we actually get to this piece of code when it does (apart
 from unit tests)


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

 ok, moved both new assigment to directory_ and full_name_ to swaps at the
 end of the method


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

 actually, it shouldn't make "" into "/", but i think rejecting it is also
 not right, i've modified it so that it takes "" literally; any directory
 that was set is removed, and you end up with a 'full' name of just the
 file name+extension (this allows for kind of the reverse of what i added
 this method for)


 > - what if new_directory has a redundant trailing '/'? e.g.
 /just/some/dir//?

 That's ignored, and imo not really harmful (it also doesn't do any changes
 to directories like {{{/just//some/dir}}})

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