BIND 10 #401: Timouts in the recursor

[BIND 10 Development]

#401: Timouts in the recursor
------------------------------+---------------------------------------------
      Reporter:  vorner       |        Owner:  vorner   
          Type:  enhancement  |       Status:  reviewing
      Priority:  major        |    Milestone:           
     Component:  recurser     |   Resolution:           
      Keywords:               |    Sensitive:  0        
Estimatedhours:  0.0          |        Hours:  0        
      Billable:  1            |   Totalhours:  0        
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by stephen):

  * owner:  stephen => vorner


Comment:

 > It was agreed that I write CZ.nic until the agreement is finished,
 > then all the headers will be changed to ISC. I'm not sure if it was
 > already signed or not, I hope I'll get notified once it is.

 Fair enough -  wasn't aware of that.

 > You mean to object member variables, right?
 >
 > I originally guessed this marks a private member, not variable and
 > since I had them all public, I didn't give them underscores. But I
 > added them.

 I'm not sure about that - I thought it was to distinguish between member
 variables and local ones.

 >> Test classes and methods should be documented to the same standards as
 >> the code being tested (so method and class headers should be added).
 > I added comments to them, but not doxygen ones (having interface of
 tests in the API
 > documentation does not seem useful).

 It's always possible to exclude the test files from the Doxygen
 documentation (see item 10 in
 http://www.stack.nl/~dimitri/doxygen/faq.html) if we want to.  But
 otherwise, if you're modifying the code you will need to modify the test
 code as well so may well want to see the documentation for it.  However,
 leave it as is for now.

 >> In TEST_F(UDPQuery, stop), it is not clear why two calls to
 service.post()
 >> are being made. The check only tests that the callback function is
 called,
 >> and does not seem to distinguish the effects of the two calls.
 > I tried to explain it in a comment. The second post is not for query,
 but for
 > stop() ‒ I test stopping the query both after it got called and before.

 OK - I got confused by the fact that there is a UDPQuery class in the .cc
 file and one in the test file - I was looking at the wrong one when
 following the logic.

 >> Using a shared_ptr is a good idea to minimise the overhead of copying.
 However,
 >> "Priv" is not a very descriptive name for the structure (I thought it
 was short
 >> for "privilege"). I would suggest something like "!UdpQueryData".
 > I changed it to !PrivateData? (!UdpQueryData? seems too much
 duplicating, as it is
 > inside UDPQuery class already).

 Fine.

 > Everything else should be done now. Do you think it is clean to sync
 before merge?

 Yes.

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