BIND 10 #3075: Implement Main Event Loop in D2Process

[BIND 10 Development]

#3075: Implement Main Event Loop in D2Process
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:  Task# 6.2.1   |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130904
         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 c39eb9bbe30285a2b19fea86473b63ddb758506b
 >
 > The code has a very good quality and it is very well documented. Thanks
 for that.
 >

 Thank you, kindly.

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

 Good point, I did not offer a !ChangeLog entry. How about something like
 this?

 6xx.    [func]  tmark
     Added main process event loop to D2Process which is the primary
     application object in b10-dchp-ddns. This allows DHCP-DDNS
     to queue requests received from clients for processing while
     listening for cmd control events.


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

 I understand what you are describing and don't disagree with you.  I did
 not pick up on that particular seperation of function between build and
 commit.  If you look through d2_config.cc, you will see that I followed
 the same pattern throughout.  I am willing to rework them as part of a
 separate ticket, specifically to address your issue. I would prefer not to
 do a large scale restructure of the parsing within this ticket (3075).


 > TSIGKeyInfoParser::commit: typo ''Algorithme''

 Fixed.

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

 Fixed.

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

 Fixed. I had little script around the reorder_message_file.py that was
 echoing out the directory.

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

 Done.

 >
 > Typo ''accpeting'' in DHCP_DDNS_QUEUE_MGR_RESUMING.

 Fixed.

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

 Done.

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

 This method is actually called by DController::initProcess.  This method
 is responsible for instantiating the process under its control and then
 invoking this method.  I kept it apart from the constructor thinking that
 constructors are not necessarily the place to attempt opening IO or other
 resources.  Keep in mind the DController/DProcess were intended to be
 general purpose.

 When I initially developed them I created this to allow for flexibility.
 I
 have amended the header commentary.

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

 Turned it into an unsigned int.


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

 Oops. I don't know why I do that sometimes.


 > 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?


 Yes it is guaranteed to be in a "reconfigruable" state first.  The only
 calls to it are within checkQueueStatus and only when in states that
 permit it.

 Initially, it was this way but that was before I realized that stopping
 may require canceling a pending IO, and you must wait for that IO's
 "operation aborted" event to occur.  This resulted in the addition of the
 STOPPING state and the extra logic needed to transition through it.

 So, no you cannot call stopListening() from reconfigrureQueueMgr.

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

 They are just helpers but is there harm in them being public? You never
 know when somebody might want them.

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

 Agreed.


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

 Done.

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

 Oops.

 >
 > canShutdown: could this function be const?

 Apparently yes, but I had to change DProcess::shouldShutdown to const to
 do it. ;)

 >
 > getD2QueueMgr and getD2UpdateMgr: could these methods be const?

 Done.

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

 Done.

 >
 > Could these methods be private?

 These are protected so they can be called by test derivations.
 >
 > Also, the arguments in these functions could be const.
 >

 Done.

 > getShutdownTypeStr: the argument could be const.
 >

 Done.

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

 Actually, the only exception that can be thrown is a programmatic
 exception, in which case you're toast anyway.
 NameChangeListener->stopListening, intentionally does not throw.

 I have a added a try-catch around it which logs but swallows an exception.
 There isn't much of way to do anything sensible as we shouldn't be able to
 get here.


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

 Deleted CONFIG_INVALID as it was never used.  I did document the others.

 >
 > 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?
 >

 Good catch, you're right, I changed it.


 > '''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_);
 > }}}
 > ?

 It was accidental. I needed to see the exception was tossing and forgot to
 undo it.

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

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

 Fixed.

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