BIND 10 #347: Implement query counters in auth module

[BIND 10 Development]

#347: Implement query counters in auth module
------------------------------+---------------------------------------------
      Reporter:  naokikambe   |        Owner:  jinmei               
          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 y-aharen):

  * owner:  y-aharen => jinmei


Comment:

 Jinmei-san, thank you very much for you review.
 Committed to branch r4015.

 Replying to [comment:14 jinmei]:

 > > >  - 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.
 OK. I added documentation. The test seems to be unnecessary because the
 result is obvious, I removed it.

 > > > '''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.
 I see. I think it is not necessary to assert because the parameters are
 checked in setupTimer().

 > > > '''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?
 OK. I will open a ticket later.

 > '''additional comments'''
 >  - as usual, I've made some minor editorial suggestions (r3977)
 I've checked it and merged them. thank you.

 >  - 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")
 OK. I renamed them.

 >  - in asio_link.cc, this should (better) be "const asio::system_error&":
 > {{{
 > +    } catch (asio::system_error& e) {
 > }}}
 OK. I modified as you suggested.

 >  - in the following part of auth_srv_unittest.cc,
 s/IPPROTO_IP/Unexpected/?
 > {{{
 > +// Submit unexpected type of query and check it throws IPPROTO_IP
 > }}}
 Yes, it is isc::Unexpected, not IPPROTO_IP.

 If there is no problem, I will merge the branch into trunk.

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