BIND 10 #2930: Sending notifications over msgq

[BIND 10 Development]

#2930: Sending notifications over msgq
-------------------------------------+-------------------------------------
            Reporter:  vorner        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  Inter-module  |  reviewing
  communication                      |                    Milestone:
            Keywords:                |  Sprint-20130528
           Sensitive:  0             |                   Resolution:
         Sub-Project:  Core          |                 CVSS Scoring:
Estimated Difficulty:  3             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 vorner]:

 > Replying to [comment:7 jinmei]:
 > > - not sure if it's related, but cmdctl unittest fails for this branch:
 > > {{{
 > > Running test: cmdctl_test.py
 > > ....................................E
 > > ======================================================================
 > > ERROR: test_wrap_sock_in_ssl_context (__main__.TestSecureHTTPServer)
 > > ----------------------------------------------------------------------
 > > Traceback (most recent call last):
 > >   File
 "/Users/jinmei/src/isc/git/bind10-2930/src/bin/cmdctl/tests/cmdctl_test.py",
 line 741, in test_wrap_sock_in_ssl_context
 > >     BUILD_FILE_PATH + 'cmdctl-certfile.pem')
 > >   File
 "/Users/jinmei/src/isc/git/bind10-2930/src/bin/cmdctl/cmdctl.py", line
 610, in _wrap_socket_in_ssl_context
 > >     raise socket.error
 > > socket.error
 > > }}}
 >
 > I don't see any possible way how it could be related. Also, it doesn't
 fail for me. Is it repeatable? Master passes (or, the base of the branch)?

 It's reproduciable, it doesn't happen on master, and it doesn't happen
 at the branch point (which I believe is commit fededac).

 > > - are we going to describe the concept of notification in API docs?
 > >   or is there already such description?
 >
 > Which API doc's do you mean? It is described in the cc-high.txt (I
 updated little detail there now). Do you mean some other?

 You should mean ipc-high.txt.  And, I didn't mean anything specific, I
 was just asking.   Referring to ipc-high.txt is okay at least for
 now.  I also don't like to repeat the same thing in multiple places.
 While referring to a completely separate doc has its own disadvantage
 (as API and implementation evolve there can be discrepancy and it's
 much easier we forget updating the doc; doxygen output can't refer to
 the separate doc), but I don't have a very good idea of how to balance
 these requirements.

 > > - maybe missing exception cases
 >
 > I don't think I can reasonably trigger the ModuleCCSessionError. But
 even if I could, it wouldn't be testing anything in the method anyway.

 I believe it should be fairly easy if we use a mock (as we actually
 do).  And we can confirm the exception is actually propagated by a
 test.  But I don't necessarily insist it's absolutely necessary.

 On the revised branch:

 '''documentation general'''

 - (something I didn't notice in the first review) based on the
   terminology of ipc-high.txt, I believe "client(s)" should be
   "session(s)", e.g., here:
 {{{
         Send a notification to subscribed clients.

         Send a notification message to all clients subscribed to the given
         notification group.
 }}}
   also: maybe s/subscribed/subscribing/?

 - (again I didn't notice it previously) I'm not sure how configuration
   change can be delivered this way:
 {{{#!cpp
     /// \param name The name of the event to notify about (for example
     ///     `config_changed`).
     /// \param params Other parameters that describe the event. This might
     ///     be, for example, the new configuration value. This can be any
 }}}
   Is this supposed to be sent by cfgmgr to the relevant module(s)?  If
   so, I suspect cfgmgr shouldn't simply forget it after sending it: it
   needs to get a response to see whether the update succeeds (and
   whether it succeeds for all that are interested in that config).
   For the purpose of this ticket, I'd suggest using more obvious
   example for which we know we use notifications: group membership
   management.  So the above text would read:
 {{{#!cpp
     /// \param name The name of the event to notify about (for example
     ///     `new_group_member`).
     /// \param params Other parameters that describe the event. This might
     ///     be, for example, the ID of a new group member. This can be any
 }}}

 '''ccsession.py'''

 - this notify should also be non blocking, right? (there can be
   deadlock situation with multi threads, but that's another issue and
   is actually a bug we should fix in a cleaner way anyway).

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