BIND 10 #762: Conversion of xfrout to use the new logging interface

[BIND 10 Development]

#762: Conversion of xfrout to use the new logging interface
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  stephen                            |                Status:  closed
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110628
                   Priority:  major  |            Resolution:  complete
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  2.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * status:  reviewing => closed
 * resolution:   => complete


Comment:

 Replying to [comment:9 jinmei]:
 >
 > The revised version looks mostly okay except two things:
 >
 > Also, I suggest performing manual checks (for now) about the
 > consistency of .mes and xfrout.py(.in), i.e., all defined ID is used
 > and all used IDs are defined.  I've not done that comprehensive check
 > myself.
 >

 I have yesterday, just not on the arguments.

 > Other points are minor, so please feel free to address or ignore them
 > as you like, and, when done, feel free to merge.
 >

 Ok, thanks, i'll merge it and will close the ticket, but I do have some
 final comments below (submitted so we at least have them recorded
 somewhere)

 > >
 > > If we then want to make it exhaustive, perhaps we can even try to get
 the tests to error if anything was logged that was not checked. This all
 is indeed outside the scope of this ticket, but definitely worth
 considering IMO.
 >
 > These seem to be good ideas (aside from how exactly we implement them,
 > and on further thought we may thing these are overkilling).  In any
 > case that's beyond the scope of this ticket.  I see some (or all?)
 > points are covered in #1055.  If #1055 would cover everything, we are
 > done with that.  If there are some other things, I'd suggest adding
 > them as a comment to #1055 or creating a new ticket.
 >

 it would be nice to have the option to additionally check whether log
 messages are actually called (when we expect them to), and not only occur
 in the source code. But it would be 'nice to have' not 'we need it
 yesterday', so i think we can try to come up with a good way to do it
 before we start making tickets. But #1055 we need yesterday :)

 >
 > Right.  I'm not sure whether we should prepare for that in this
 > context.  We might implicitly clarify that by saying "in the current
 > implementation" or something.  I'd leave it to you.
 >

 I'll leave it as is, 'in the current implementation' has no meaning IMHO,
 unless we include a specific version; if we forget to update the text, it
 will still be wrong.

 > As for XFROUT_AXFR_TRANSFER_DONE, it seems this could be logged when
 > the transfer was incompletely terminated due to a shutdown while the
 > log description says:
 > {{{
 > +The transfer of the given zone has been completed successfully.
 > }}}
 > I'd say, e.g., "...has been successfully completed or aborted
 > halfway due to a shutdown event".
 >

 Oh, right, done.

 > > > - XFROUT_SOCKET_SELECT
 >
 > > >   - does this really indicate "an error in the internal
 communication with b10-auth"?
 > >
 > in subsequent read/write operation.  If my understanding is correct,
 > I'd replace "This most likely points to an error in the internal
 > communication with b10-auth." to, e.g. "This should be a result of
 > rare local error such as memory allocation failure and shouldn't
 > happen under normal conditions."
 >

 ok, updated

 > [snip, context is whether we need _ERROR in the ids]
 >
 > I don't like redundancy (in general:-).  My main point in this case
 > was the inconsistency (I actually suggested adding _ERROR, but at that
 > time I didn't consider the redundancy that much).  As for redundancy,
 > I see the point on one hand.  I've been also a bit concerned about
 > possible inconsistency between the defined level and description due
 > to future changes to the level.  For example, in the above example, we
 > might change our mind and make the level to "info", by changing
 > "logger.error" to "logger.info".  That would implicitly request
 > changes to the log description (and ideally the ID also for
 > consistency), but I can easily imagine the situation we forget it.
 >
 > So, it would be nicer if we only have a single definition point on the
 > log level, while still naming log IDs reasonably.  But, like other
 > points in this ticket, it would be beyond the scope of this ticket.
 > So I'm okay with the current code for this specific case.

 That was partly why i kept my name to 'context' more than 'issue' (i.e.
 logger.error(SETUP) as opposed to logger.error(SETUP_ERROR)), but just
 looking at the message file, this also looks very weird, so for now it's
 _ERROR everywhere.

 I actually think that forgetting to change the name or description if we
 change the severity level of a call is not the biggest potential problem;
 at least at that point you are modifying a log call and have a reasonable
 chance of thinking about it. I'm much more worried that, given the level
 of detail we put in these messages, we forget to update it if we change
 code behaviour or error conditions... (in which case it may be far from
 obvious if there are any relevant log messages that need updating)

 > > >
 > > > - XFROUT_REMOVE_OLD_UNIX_SOCKET_FILE: doesn't seem to be used in
 .py.
 >
 > This doesn't seem to be fixed.

 name was, argument was not, my bad

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