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

 First off, I assume you meant to pass this ticket back to me based on
 the comments and code changes.  If it was indeed the case, please make
 sure to change the ticket owner.  Basically, as long as you own the
 ticket the reviewer would assume you're still working on it and
 wouldn't resume the review.

 See also: http://bind10.isc.org/wiki/CodeReviewProcedure

 Replying to [comment:10 fujiwara]:

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

 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.

 I didn't see any problem to understand what this branch does for what
 purposes without looking into 'stats201209-auth-merge', so this
 "please refer to" comment is simply unclear.

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

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

 Hmm, apparently I forgot to push my local changes.  I've merged your
 fixes with mine, with some additional editorial changes, and pushed
 them.

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

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

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

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

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

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