BIND 10 #640: cfgmgr hanging and command and configurations for no-longer-running components

[BIND 10 Development]

#640: cfgmgr hanging and command and configurations for no-longer-running
components
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  UnAssigned
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:  major  |  Sprint-20120207
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I've not yet reviewed everything, but as I'm taking a day off tomorrow
 I'll provide feedback on the things I've covered so far.

 First off, it was not clear which part of the original report was
 solved.  I see the potential that this set of changes can make bindctl
 more responsive in the reported scenario, but was it identified why
 cfgmgr was hanging (or did it really hang)?  And does this branch
 solve that problem?  What about the traceback output?  (BTW I
 understand this branch cannot solve the issue of configuring inactive
 component).

 A related point is that since this is an issue concerning interactions
 of multiple processes, unittests wouldn't be enough to be sure whether
 the problem was solved.  It may be okay to defer it to a separate
 ticket, but I think we need a system level test for this kind of
 thing.

 Next, I made a few trivial changes to the branch.

 '''ccsession.cc'''

 - Not a big deal, but `sendStopping()` is a private method, not very
   big, and used only within the ModuleCCSession destructor.  Is it
   worth a separate method?
 - logging itself could cause an exception, so the destructor could
   still throw.  However, this is not the only point that has this
   problem, so maybe we can leave it open for now...

 '''ccsession.h'''

 - I thought we decided to use C++ style comments for doxygen
 - In our convention we'd explicitly add 'virtual' (for clarity) to the
   destructor declaration.

 '''config_message.mes'''

 If not yet done, you may want to reorder the messages using
 tools/reorder_message_file.py

 '''ccsession_unittests.cc'''

 - maybe a matter of taste, but the intermediate block just to have a
   separate scope looks a bit awkward.  I'd do something like this:
 {{{#!c++
     scoped_ptr<ModuleCCSession> mccs(new ModuleCCSession(
                                          ccspecfile("spec2.spec"),
                                          session, NULL, NULL,
                                          true, false));
     ...
     mccs.reset();               // this will invoke destructor
 }}}
   At least I think some comments about the intent of the scope would
   be necessary.
 - it would be nicer if we could test the case where the destructor
   throws.

 '''ccsession.py'''

 - send_stopping: while the pydoc says "any errors are logged", only
   SessionError is caught.  Is that intentional?  Can we assume
   create_command() doesn't raise an exception?
 - send_stopping: I believe you don't need 'str' for 'se'.  (might not
   be a big deal in this case, but delaying 'str' could possibly help
   improve performance).

 '''cfgmgr.py'''

 - if `_handle_module_stopping` is private, should it better be
   prefixed with `__`?  Same for `_send_module_spec_to_cmdctl`.
 - `_handle_module_stopping` returns None, saying "This command is not
   expected to be answered".  But returning None doesn't seem to
   suppress sending a response.
 - Not directly related to this ticket, but the if-else in handle_msg
   is getting too big to understand/manage.  I guess it's time to
   consider refactoring, maybe in some OO way.

 '''ccsession_test.py'''

 - test_stop: not necessarily a problem, but it seems the same pattern
   repeats in multiple tests.  It would be better to unify the common
   sequence of tests for conciseness.

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