BIND 10 #744: Conversion of data source library to use the new logging interface

[BIND 10 Development]

#744: Conversion of data source library to use the new logging interface
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  minor  |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  logging                            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => vorner


Comment:

 '''src/lib/datasrc/Makefile.am'''[[BR]]
 As noted in the [ticket:745#comment:7 response] to the review of #745, the
 generated files (messagedef.{cc,h}) should be put in the
 nodist_libdatasrc_la_SOURCES variable.

 '''src/lib/log/compiler/message.cc'''[[BR]]
 Although in that response I said that I don't know why changing the
 message compiler caused "make distcheck" to work, looking at the change
 more closely I think I now have an idea.

 I would hazard a guess that it was due to the Makefile.am for #745 only
 giving the name of the message file on the message compiler command line,
 not the full file specification (which includes the directory) as in the
 Makefile.am in this ticket.  This would mean that the .cc and .h files
 were created in the default directory in ticket #745, whereas here they
 were created in the same directory as the .mes file.  The two are
 different in a "make distcheck" which may have been the problem.  The
 change made to the message compiler in this ticket ensures that the output
 files are created in the default directory (which is the correct
 behaviour) and so would give the behaviour seen in #745.

 However, the explanation does presuppose that the message compiler had
 been built at some point.  A "make distcheck" done on a clean system fails
 to copy the created .cc and .h files because of its absence (as explained
 in the [ticket:745#comment:7 response] to the review of #745).

 '''src/lib/datasrc/messagedef.mes'''[[BR]]
 Given the length of the identifiers, I suggest that CACHE_CREAT be renamed
 to CACHE_CREATE - there seems no reason to suppress the final "E".

 General: a number of debug messages omit the name of the item concerned.
 Although in many cases they refer back to a previous message (e.g.
 CACHE_OLD_FOUND refers to the item name in a previous CACHE_LOOKUP), I
 think debugging would be simpler if the data were included in the message.
 If nothing else, a future change to the code could separate the two
 messages with other ones (and if that change were a recursive call, it
 would really confuse things).

 General: rather than than use the first person in explanations ("we're
 adding a SOA record for the given zone into the authority section"), I
 think third-person sounds better in what will ultimately be a manual (i.e.
 "An SOA for the given zone is being added to the authority section").

 General: suggest that the message text start with a lower-case character
 (unless something that should be in capital letters such as the type of a
 resource record).  The reason is that (the current) message formatting is
 {{{
 MESSAGE_ID, message text
 }}}
 ...and a lower-case letter after the comma follows the rules of English.

 QUERY_SYNTH_CNAME: suggest changing "info." to "information" to be
 consistent with other message descriptions.

 QUERY_SIMPLE_FAIL: should be "...should have logged..."

 MEM_WILDCARD_DNAME: should be "... BIND 9 ..." (BIND should be in capital
 letters).

 QUERY_ADD_RRSET: should be consistent and use "RRset", not "rrset".


 '''src/lib/datasrc/cache.cc'''[[BR]]
 HotCache::retrieve(): suggest that the the CACHE_LOOKUP message be removed
 and that the name of the question be added to the CACHE_NOT_FOUND,
 CACHE_FOUND and CACHE_EXPIRED messages instead.  One of those three must
 be output if tracing is enabled, so they can be used to check if
 retrieve() has been called.  And it means that only one message instead of
 two is output for every retrieval.

 HotCacheImpl::!SetSlots(): is it useful to add to the CACHE_SLOTS message
 the number of entries that will be removed when the size of the cache is
 set?

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