BIND 10 #347: Implement query counters in auth module

[BIND 10 Development]

#347: Implement query counters in auth module
------------------------------+---------------------------------------------
      Reporter:  naokikambe   |        Owner:  y-aharen             
          Type:  enhancement  |       Status:  reviewing            
      Priority:  major        |    Milestone:  y2 12 month milestone
     Component:  b10-auth     |   Resolution:                       
      Keywords:               |    Sensitive:  0                    
Estimatedhours:  0.0          |        Hours:  0                    
      Billable:  1            |   Totalhours:  0                    
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => y-aharen


Comment:

 Replying to [comment:13 y-aharen]:

 The latest code looks almost ready for merge.  I've replied to some
 selected points that may not be trivial.  I also added some new (not so
 substantial) comments.  Other points are okay and simply ommitted.

 > >  - IntervalTimer destructor: my previous question doesn't seem to be
 > >   addressed in the documentation: "what happens if we destruct
 !IntervalTimer before starting a timer?"
 > If we destruct it before starting a timer, the callback function should
 not be called. I wrote a test.
 >
 I intended to ask to add a note in the documentation (which is not done
 yet, right?), but of course adding a test is good, too.  Regarding the
 test, however, I'm not sure if it tests the right thing.  I guess the new
 test for this is deleteIntervalTimerBeforeStart, but since it doesn't
 start the io_service_, it should be obvious that timer_called is
 unchanged.  And, I guess the destructIntervalTimer actually tests what
 we'd want to test in this context.

 > >  - setupTimer(): like the constructor, asio::system_error shouldn't be
 exposed.
 > I wrote try-catch to convert asio::system_error to isc::Unexpected and
 added documentation. It is hard to modify the library to throw an
 exception, so I didn't wrote a test.
 >
 Okay.

 > > '''asio_link.cc'''
 > >  - IntervalTimerImpl::callback()
 > > > OK. IntervalTimerImpl::callback() no longer exists. setupTimer()
 checks
 > > > its arguments.
 > >
 > > I guess you mean "no longer exits (or asserts)"?  BTW with the check
 > > in setupTimer() I'm okay with the assert() (or may even be a good
 > > practice, but it's up to you).
 > It means I removed assertion from IntervalTimerImpl::callback(). I think
 throwing an exception is suitable for parameter check, so the code throws
 an exception.
 >
 Throwing an exception in setupTimer() is fine.  I meant the assert() in
 callback(), and now the setupTimer() ensures the parameters are valid and
 it's the only place they can be set, we can safely assert() the fact here.
 But this is just a comment; whether to re-introduce the assert() is up to
 you.

 > > '''asio_link_unittest.cc'''
 >
 > >  - the additional tests introduce another 10 seconds of delay.  in
 > >  general, imposing a long delay in unit tests is not a good practice
 > >  as unit tests are supposed to be performed "instantly".  These types
 > >  of delay have been introduced in our tests already (some of them were
 > >  introduced by myself), and so it's not specific to the timer tests,
 > >  but 10 seconds still look quite substantial.  I'd not necessarily
 > >  request it be solved right now, but the timer should eventually have
 > >  finer granularity for the timer duration and use shorter timer
 > >  periods for tests.  For the moment I suggest adding some notes about
 > >  this point in comments.
 > I noted these tests takes long time and this issue should be addressed
 in future.
 >
 Okay.  Could you open a separate ticket about this so that we can keep
 truck of it?

 '''additional comments'''
  - as usual, I've made some minor editorial suggestions (r3977)
  - as for QueryType, I guess it's now better be named (e.g.) CounterType
 now that the class name is not specific to queries.
  - likewise, s/COUNTER_UDP/COUNTER_UDP_QUERY/? (imagine we'll want to
 define a counter type for e.g. "UDP notify")
  - in asio_link.cc, this should (better) be "const asio::system_error&":
 {{{
 +    } catch (asio::system_error& e) {
 }}}
  - in the following part of auth_srv_unittest.cc,
 s/IPPROTO_IP/Unexpected/?
 {{{
 +// Submit unexpected type of query and check it throws IPPROTO_IP
 }}}

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