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:  fujiwara
  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 fujiwara):

 Replying to [comment:8 jinmei]:
 > > For reviewing, please also refer to branch 'stats201209-auth-merge'.
 > I don't understand what this means.  Could you elaborate?

 The branch is only for testing.

 > '''general'''
 > - I believe we need a changelog entry for this ticket.

 ticket 2136 will add a changelog entry which describes ticket 2136, 2137
 and 2138.

 > - It looks like we need to update the BIND 10 guide too.
 updated

 > - 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 removed the code because ticket2136 removes Stats side code which
 receives
 spontaneous update of statistics data. We can re-add it later if we need.

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

 The reason of system tests failure is that trac2138 changed only auth
 server.
 Changes by trac2136 is required for running system test.

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

 "getstats" command uses normal command interface. "sendstats" used another
 communication channel.

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

 changed as "/// \return command result using createAnswer()."

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

 recovered.

 > - 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.
 used the code.

 > '''statistics.cc'''

 > - 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).
   changed as "static_cast<isc::data::ConstElementPtr>(statistics_element)"

 > '''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.
 > }}}
   changed as "Just check some message has been received."

 > '''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.
 tried to test with server.setListenAddresses() and
 server.getListenAddress().
 It may be incomplete.

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