BIND 10 #2138: update the Auth module according to the new statistics model

[BIND 10 Development]

#2138: update the Auth module according to the new statistics model
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  UnAssigned
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120821
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  b10-auth                           |  Estimated Difficulty:  6
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 y-aharen]:
 > For reviewing, please also refer to branch 'stats201209-auth-merge'.

 I don't understand what this means.  Could you elaborate?

 Anyway, the following are my review comments on the branch (I've not
 looked into the stats201209-auth-merge branch).

 '''general'''

 - I believe we need a changelog entry for this ticket.
 - It looks like we need to update the BIND 10 guide too.
 - I thought auth will still need to send statistics spontaneously, in
   case it has a potentially large amount of statistics data, such as
   in the case of enabling per-zone statistics, and the stats module
   doesn't try to get it for a while.  So I'm not sure if it's a good
   idea to remove the code related to this behavior completely.  What's
   the plan about it?  Are we going to re-add it later?
 - I've seen some long lines and fixed them.  Please make sure the
   style guideline on the expected max number of columns (79) is met.
 - some system tests now fail.  Please fix them.

 '''auth_messages.mes'''

 - AUTH_RECEIVED_GETSTATS: why did you remove the second sentence?
 {{{#!diff
 -a command from the statistics module to send it data. The 'sendstats'
 -command is handled differently to other commands, which is why the debug
 -message associated with it has its own code.
 +a command from the statistics module to send it data.
 }}}
   I really don't know what "handled differently" means, but if the
   reason is that it's not handled differently any more, it seems to me
   we can remove the special log at all, rather than renaming it.

 '''command.cc'''

 - AuthCommand::exec(): please describe the requirement for derived
   class implementations about the return value in the detailed
   description part.  'command result data' is not really clear.
   Assume someone writing a new command class, only referring to this
   description (not looking into other derived class implementation).
   We need to provide sufficient information for such developers.
 - why did you remove the second sentence?
 {{{#!diff
 -// Handle the "shutdown" command. An optional parameter "pid" is used to
 -// see if it is really for our instance.
 +// Handle the "shutdown" command.
 }}}
 - execAuthServerCommand: we can return the exec result within the try
   block and avoid defining the 'value' variable
 {{{#!cpp
 {
     LOG_DEBUG(auth_logger, DBG_AUTH_OPS,
 AUTH_RECEIVED_COMMAND).arg(command_id);
     try {
         return (scoped_ptr<AuthCommand>(
                     createAuthCommand(command_id))->exec(server, args));
     } catch (const isc::Exception& ex) {
         LOG_ERROR(auth_logger, AUTH_COMMAND_FAILED).arg(command_id)
                                                    .arg(ex.what());
         return (createAnswer(1, ex.what()));
     }
 }
 }}}
   This may be a matter of taste for such a small function, but I
   generally prefer avoiding a (especially mutable) variable if we can
   easily do so because they are often a source of bugs.

 '''statistics.cc'''
 - redundant spaces in building statistics_string looks awkward.  I've
   fixed them.
 - It doesn't seem to be good that we cannot pass `ConstElementPtr` to
   the validator (forgetting that I don't even think it makes sense to
   try to validate it in the sender side):
 {{{#!cpp
     isc::data::ElementPtr statistics_element =
         isc::data::Element::fromJSON(statistics_string);
     // validate the statistics data before send
     if (validator_) {
         if (!validator_(statistics_element)) {
 }}}
   And, in fact, the new version of the code slightly modified the
   semantics because the validator_ could modify the passed data and we
   could now return it (previously we passed a copy to the validator_
   and always use the build data whatever the validator does to the
   given data).  If it's really the case, we should update the auth
   side code so we pass a copy; if it's not the case, we should
   probably revisit the validator interface so we can pass a const
   pointer (although it would be beyond the scope of this ticket).

 '''command_unittest.cc'''

 - getStats test: the comment seems to be a naive copy-paste.  It's
   different from what's actually happening.
 {{{#!cpp
     // Just check some message has been sent.  Detailed tests specific to
     // statistics are done in its own tests.
 }}}

 '''config_unittest.cc'''

 - exceptionGuarantee: I suspect you cannot simply remove this test.
   This looks like testing the exception guarantee in general, not
   specific to the statistics timer interval.  With the removal of this
   config, we need to find another config parameter to test the
   guarantee.

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