BIND 10 #2922: enhance group subscriber management in msgq

[BIND 10 Development]

#2922: enhance group subscriber management in msgq
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |                       Status:
            Priority:  high          |  reviewing
           Component:  msgq          |                    Milestone:
            Keywords:                |  Sprint-20130611
           Sensitive:  0             |                   Resolution:
         Sub-Project:  Core          |                 CVSS Scoring:
Estimated Difficulty:  4             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by pselkirk):

 * owner:  pselkirk =>


Comment:

 '''src/bin/bind10/init.py.in'''
 You have a couple of overly-long lines, but that's a problem in other
 parts of the file as well.
 I'm also not fond of the indented method documentation, but that seems to
 be mostly consistent throughout the module.

 I'm also not sure about the timing in `wait_msgq`. If I understand
 correctly, it sends up to 10 messages at 100ms intervals, then gives up.
 Will this cause problems on older/slower hardware, or say if bind10 is
 starting up at the same time as everything else on the system?

 '''src/bin/bind10/tests/init_test.py.in'''
 I'm not sure why you import `ProcessStartError`, since the only other
 place it's referenced in this module, it's qualified with `init`.

 This also means you've got this line:
 {{{
         self.assertRaises(ProcessStartError, init.wait_msgq)
 }}}
 where both `ProcessStartError` and `wait_msgq` are from `init`, but one is
 qualified and one unqualified.

 '''src/bin/msgq/msgq.py.in'''
 I had some documentation nits, which I just fixed directly and committed.

 '''src/bin/msgq/tests/msgq_run_test.py'''
 In `MsgqRunTest.test_notifications` you say:
 {{{
         The test is here, because there might be some trouble with
 multiple
         threads in msgq which would be hard to test using pure unit tests.
 }}}
 What sort of trouble? Is this something we should be testing for?

 This test initializes `attempts` to 100, then ignores it - it just spins
 until it receives a response. This looks like a bug.

 As an aside, when I see lines like this:
 {{{
         self.__msgq.process_command_subscribe(self.__msgq.lnames["first"],
                                               {'group': "G1", 'instance':
 "*"},
                                               None)
 }}}
 I scratch my head about the usage of single vs double quotes.

 At a guess, you're using single quotes for keys or labels, and double
 quotes for values. OTOH, in `test_notifies`, you use single quotes
 exclusively. This isn't addressed in either the BIND 10 style guide or the
 Python style guide.

 Nit: in `test_get_members`, you have this:
 {{{
         # We pretend that all the possible groups exist, just that most
         # of them are empty. So requesting for G3 is request for an empty
         # group and should not fail.
         self.assertEqual({'result': [0, []]},
                          self.__msgq.command_handler('members',
                                                      {'group': "Empty"}))
 }}}
 Comment says "G3", code says "Empty".

 '''src/bin/msgq/tests/msgq_test.py'''
 `MsgQTest.test_notifies` should probably test unsubscription from a group
 not prevously subscribed to.

 `test_notifies` and `test_notifies_implicit_kill` have some ugly code to
 "Steal the lname". Is there a reason you can't use `fd_to_lname`?

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