BIND 10 #3052: Create QueueMgr class for D2

[BIND 10 Development]

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

 * owner:  marcin => tmark


Comment:

 Reviewed commit f964dbc5497ea2aef884251c708007e97185df2e.

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

 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?

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

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

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

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

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

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

 peekAt: could index be const?

 dequeueAt: could index be const?



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


 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.

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


 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.

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


 Any plans for !ChangeLog?

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