BIND 10 #3052: Create QueueMgr class for D2

[BIND 10 Development]

#3052: Create QueueMgr class for D2
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130731
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

 Replying to [comment:6 marcin]:
 > Reviewed commit f964dbc5497ea2aef884251c708007e97185df2e.

 Comments addressed with git ce9289d1ef31525a60dccbe0daa72e8f8db1be63

 >
 > '''d2_queue_mgr.h'''
 > !D2QueueMgrError: typo !''an general!''. Should be !''a general!''
 >
 Got it.

 > D2QueueMgr: I remember us discussing replacement of !''Que!'' (also
 !''flushQue!''). The doxygen description of the class says: !''...via the
 flushQue() method.!''. Shouldn't be corrected?
 >
 The method is called, clearQueue, and I believe I replaced all occurences
 of "que".


 > BTW, the description of the class is meaningful. Thanks for this.

 Your welcome.

 >
 > Shouldn't !QueueMgr be declared !''noncopyable!''?

 I suppose it could be to be on the safe side.  Done.

 >
 > Please correct the spacing between the constant name and the rest in:
 > {{{
 >     static const size_t MAX_QUEUE_DEFAULT=  1024;
 > }}}

 Oops. Fixed.

 >
 > D2QueueMgr constructor: I believe there is a typo in the @throw tag. It
 should rather be:
 > {{{
 >     /// @throw D2QueueMgrError if max_queue_size is zero.
 > }}}

 Oops again. Fixed.
 >

 > ~D2QueueMgr: It is a neat pick. But, the base class destructor is
 declared virtual so child class destructor is assumed virtual. Therefore,
 this destructor doesn't need an explicit !''virtual!'' keyword. However, I
 would put it here anyway, because if you ever decide that this class
 shouldn't derive from its base class, you will likely forget to add
 virtual.
 >

 Good point, done.

 > initUDPListener: I think there is no need to pass the !''port!'' by
 reference. The other two arguments could be declared const.
 >
 Agreed, done.


 > peekAt: could index be const?
 >

 Yes. Done.

 > dequeueAt: could index be const?

 Yes. Done.
 >
 >
 >
 > '''d2_update_mgr.cc'''
 > Destructor calls the stopListening which is not exception safe.
 Admittedly, the current implementation of stopListening does not throw an
 error if stopped_state == STOPPED but if the implementation of this
 function changes... it may. I would prevent that in destructor  today,
 because it is much harder to remember about such things in the future.
 Throwing exceptions from destructor is generally discouraged because
 cleanup may not be completed and may cause objects to hang.
 >

 I added a try/catch as a safety net, but did not bother with logging
 anything.

 >
 > peekAt: The exception string could include the exact values of the index
 and the queue size.
 >
 > setMaxQueueSize: The exception string could include the value of maximum
 queue size.
 >

 Added to both.

 > '''d2_queue_mgr_unittests.cc'''
 > General comment with respect to naming of test cases: the postfix
 !''Tests!'' is redundant in the test names. For example:
 !''constructionTests!'' could be simply called construction, because each
 of the test cases is prefixed with the test fixture class name anyway. For
 example: !''D2QueueMgrBasicTest.construction!'' instead of
 !''D2QueueMgrBasicTest.constructionTests!''.
 >
 >

 Yes, I'd thought this before but had started a pattern so was sticking
 with it. Have changed the names per your suggestion.



 > constructionTests: Everytime the new QueueMgr object is allocated it
 should be enclosed in ASSERT_NO_THROW rather than EXPECT_NO_THROW. This is
 because, if the construction fails, calling the method on the object will
 result in assertion. The assert would exit the test.
 > The same issue applies to all other tests.

 Stephen's rule of thumb for this is that if the remainder of test cannot
 pass,
  use an ASSERT.  I am not always dilligent about checking this especially
 when
 enerating lots of new tests.

 In the case of constructionTest, I have left them as EXPECTs.  That way
 you can see if all variant fail or only some.  Each construction scenario
 stands on it's own.

 I did fixt the other tests.

 >
 > Neat pick: Why i is enclosed in brackets here:
 > {{{
 >         EXPECT_EQ(VALID_MSG_CNT-(i), queue_mgr->getQueueSize());
 > }}}
 >

 I have no clue why I did this.  Overzealousness?

 >
 > Any plans for !ChangeLog?
 >
 >
 Please see original submission comments, just below the diagram.

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