BIND 10 #3075: Implement Main Event Loop in D2Process

[BIND 10 Development]

#3075: Implement Main Event Loop in D2Process
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:  Task# 6.2.1   |  Sprint-DHCP-20130904
           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 c39eb9bbe30285a2b19fea86473b63ddb758506b

 The code has a very good quality and it is very well documented. Thanks
 for that.

 I think it requires a !Changelog. I am hoping I haven't simply missed it
 in the ticket again.

 Other comments:

 '''src/bin/d2/d2_config.cc'''
 Invalid alignment of the second line in the constructor definition:
 {{{
 TSIGKeyInfoListParser::TSIGKeyInfoListParser(const std::string& list_name,
                                        TSIGKeyInfoMapPtr keys)
 }}}

 build and commit: The concept of the separation of build and commit
 methods is that the error prone part should be done in build() and the
 commit() should be exception free (see description of abstract methods:
 commit and build in src/lib/dhcpsrv/dhcp_config_parsers.h). The commit()
 could only do:
 {{{
     *keys_ = *local_keys_;
 }}}
 Everything else should be moved to the build(). In fact, similar changes
 should be done for !TSIGKeyInfoParser.

 TSIGKeyInfoParser::commit: typo ''Algorithme''

 '''src/bin/d2/d2_config.h'''
 Doxygen description of commit has a typo ''This causes each parser to
 instantiate a TSIGKeyInfo from its internal data values and add '''that
 that''' key instance to the local key storage area, local_keys_. ''. Also
 spurious space after ''local_keys_''.

 '''src/bin/d2/d2_messages.mes'''
 Garbage text in the first line:
 ''/Users/tmark/ddns/build/new3075/bind10''.

 DHCP_DDNS_QUEUE_MGR_RECONFIG - this could be renamed to
 DHCP_DDNS_QUEUE_MGR_RECONFIGURING. This is simply because other messages
 are called DHCP_DDNS_QUEUE_MGR_RESUMING and
 DHCP_DDNS_QUEUE_MGR_RECOVERING.

 Typo ''accpeting'' in DHCP_DDNS_QUEUE_MGR_RESUMING.

 '''src/bin/d2/d2_process.cc'''
 General comment: There are multiple switch constructs in the file, which
 lack the default case. You may get yourself into trouble with some
 compilers complaining about it. The default case should be:
 {{{
 default:
     ;
 }}}
 where the semi colon indicates empty statement.


 init: Wouldn't it make sense to call this method when the object is
 instantiated? I know it is empty at the moment, but if you already call
 it, I don't have to bother thinking where I should call it from.
 Especially, that I am not an expert of D2Process class, because I haven't
 created it. If method is not called, it doesn't make sense to define it.

 QUEUE_RESTART_PERCENT: Could this value be declared integer with the
 allowed range of vaues between 0..100? Note that floats are not for free,
 and actually the range you're using is limited to 0..1.

 runIO: it should be
 {{{
 return (cnt);
 }}}

 checkQueueStatus: Is it always guaranteed by the queue manager state
 transitions that the queue manager is stopped before
 ''reconfigureQueueMgr'' is called? If it isn't the reconfiguration will
 result in exception. Shouldn't reconfigureQueueMgr cal the
 {{{
 queue_mgr_->stopListening();
 }}}
 by itself?

 '''src/bin/d2/d2_process.h'''

 Is there any reason why text labels for shutdown types are declared
 '''public''' in the  header file? Will they be used externally or they are
 just helper constants for the internal use of the !D2Process class?

 Constructor: I suggest that you avoid too use of !''process!'' when it may
 be confused with the !''application process!'' term. This sentence: ''The
 '''construction process''' creates the configuration manager, the queue
 manager, and the update manager.'' could be replaced with: ''The
 '''constructor''' creates the configuration manager, the queue  manager,
 and the update manager.''

 !ShutdownType should be better documented. Suggest that you add a brief
 description which would explain differences between all types.

 run: IN the doxygen documentation you should use @code tags, not the wiki-
 style code markers.

 canShutdown: could this function be const?

 getD2QueueMgr and getD2UpdateMgr: could these methods be const?

 setReconfQueueFlag and setShutdownType: Use doxygen-style @note tag
 instead of NOTE.

 Could these methods be private?

 Also, the arguments in these functions could be const.

 getShutdownTypeStr: the argument could be const.


 '''src/bin/d2/d2_queue_mgr.cc'''
 operator() is not guarded from exceptions, which may be potentially thrown
 by the stopListening. Note that we expect this operator to be exception
 free.

 '''src/bin/d2/d_process.h'''
 This declaration:
 {{{
 static const int CONFIG_INVALID = 1;
 }}}
 deserves a comment (description), as well as other constants defined in
 this header.

 Typo in: '' '''a the''' application name string.''


 '''src/bin/d2/dhcp-ddns.spec'''
 I am not certain, but it looks to me that ''Shuts down '''b10-dhcp-ddns
 module''' '' would be more appropriate for the command_description. Is
 DHCP_DDNS a server? in some sense it is, but it may be considered as a
 part of the DHCPv6 server, so using the term DHCP_DDNS server is a little
 confusing. How do you think?

 '''src/bin/d2/tests/d2_cfg_mgr_unittests.cc'''
 Has this change been intended or accidental:
 {{{
 +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc
 @@ -347,7 +347,8 @@ TEST_F(TSIGKeyInfoTest, invalidTSIGKeyList) {
      ASSERT_NO_THROW(parser.reset(new TSIGKeyInfoListParser("test",
 keys_)));

      // Verify that the list builds without errors.
 -    ASSERT_NO_THROW(parser->build(config_set_));
 +    //ASSERT_NO_THROW(parser->build(config_set_));
 +    parser->build(config_set_);
 }}}
 ?

 '''src/lib/dhcp_ddns/dhcp_ddns_messages.mes'''
 Garbage before the copyright header:
 ''/Users/tmark/ddns/build/trac3075/bind10''

 '''src/lib/dhcp_ddns/ncr_io.h'''
 isIoPending: typo in ''Returns true if the listener '''is has''' an IO
 call in progress.''

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