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-20120904
                   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:11 jinmei]:
 > This doesn't address my question...my point is that it's not clear
 > what the reviewer should do by merely being told "please also refer to
 > XXX".  If that branch isn't necessary for reviewing the review target,
 > it should be clarified; if some part of 'stats201209-auth-merge' needs
 > to be reviewed within this ticket, it should be clarified; if some
 > part of 'stats201209-auth-merge' are considered to be needed to check
 > to understand this branch, it should be clarified, etc.
 Please ignore 'stats201209-auth-merge' branch in this review.

 > > > '''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.
 > Adding the changelog with #2136 is okay, but IMO the changelot text
 > should be proposed and reviewed here, because the developer and the
 > reviewer for that particular change should know it best.

 added changelog entry for tickets 2136, 2137, 2138.

 > > > - 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.
 >
 > In that case please make sure merging to master won't cause system
 > test regression, either by temporarily disabling some tests or by
 > carefully organizing merge order.

 Merging trac2136,2137,2138 should be done in one time, I think.

 > > > '''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.
 >
 > This doesn't address my (real) point, which was: if there's now no
 > reason for having the AUTH_RECEIVED_GETSTATS message code separately
 > from generic ones, IMO we should rather remove the code.

 Removed the message and logging when auth processes "getstats" command.

 > > > '''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()."
 >
 > This doesn't seem to be sufficient.  I committed my suggested change
 > to the branch and pushed it at commit f02efa8.  Please check it.
 Thanks.

 > > > '''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):
 > [...]
 > >   changed as
 "static_cast<isc::data::ConstElementPtr>(statistics_element)"
 > Sorry, I realized I misunderstood the original code.  I thought the
 > validator expects a mutable version of pointer, but the real issue was
 > getStatistics() was defined to return a mutable pointer.  On further
 > look I noticed it can actually be a const pointer if we change all of
 > them for the getStatistics variants to const.  I made the change
 > myself at caa70d6.
 >
 > BTW: conversion from non const to const pointer can be done
 > implicitly (and always safe), so you don't need the above cast anyway.
 Thanks, the change is clear.

 > > > '''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."
 >
 > It still doesn't seem to be correct to me because in this test
 > scenario no message was "sent" or "received".  I've committed my own
 > suggestion at 9463f3e.
 >
 > > > '''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.
 >
 > I've revised the test and committed it so configureAuthServer() will
 > actually modify partially the listen_list and rollback to the original
 > value.  The previous one was not really a good test because it didn't
 > even temporarily change the server state.  Note: using the listen_on
 > may not actually be a good test case for exception guarantee anyway,
 > because rolling back to previous port is generally difficult and
 > I suspect we'll eventually have to revisit the behavior (then the test
 > won't pass at that point).  But right now I see no other choice, so I
 > suggest we keep this test case for the moment.

 Thanks.

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