BIND 10 #2856: memory manager initialization

[BIND 10 Development]

#2856: memory manager initialization
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:  muks
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  shmem         |                    Milestone:
  manager                            |  Sprint-20130723
            Keywords:                |                   Resolution:
           Sensitive:  0             |                 CVSS Scoring:
         Sub-Project:  DNS           |              Defect Severity:  N/A
Estimated Difficulty:  4             |  Feature Depending on Ticket:
         Total Hours:  0             |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hello

 First, the scope of the ticket seems strange to me. There seems to be an
 update to builder part and also to the `SegmentInfo` class. But I don't
 think these are directly related (eg. there'll be something in between
 connecting them together, which is not there yet). I don't think the
 ticket mentions that, but it makes me ask why it is the case. Or, it is
 possible I look incorrectly into the code.

 Also, the semantics of the `SegmentInfo` class seems strange to me. It
 looks like a lot of different methods return the event to be handled. So
 the caller will have to check the return value in like 5 different places.

 The distcheck fails:
 {{{
 ERROR: files left in build directory after distclean:
 ./src/lib/python/isc/memmgr/tests/zone-IN-1-MasterFiles-mapped.0
 make[1]: *** [distcleancheck] Error 1
 make[1]: Leaving directory `/tmp/bind10/bind10-3/bind10-20130529/_build'
 }}}

 Should this at least log the error?

 {{{#!python
     def __handle_bad_command(self):
         # A bad command was received. Raising an exception is not useful
         # in this case as we are likely running in a different thread
         # from the main thread which would need to be notified. Instead
         # return this in the response queue.
         self._response_queue.append(('bad_command',))
         self._shutdown = True
 }}}

 I don't know if it's a good idea to leave the `FIXME: log error` in the
 code. It still seems better to log at least something and leave fixup of
 the messages for later, than not log at all.

 Do we really start at `READY` in the `SegmentInfo` class? Won't we start
 by loading something and sending updates to the clients?

 I don't see what is meant by „publicly available“ here:
 {{{#!python
         # __old_readers is a set of 'reader_session_id' private to
         # SegmentInfo for write (update), but publicly readable. It can
         # be non empty only in the SYNCHRONIZING state, and consists of
         # (ID of) reader modules that are using the old version of the
         # segments (and have to migrate to the updated version).
         self.__old_readers = set()
 }}}

 There's a class that does a queue effectively in python (sorry, I don't
 remember the name now). When using list, it needs to copy all the items to
 the left when removed from the head.

 You seem to check for emptiness with `len` at many places. This looks
 ineffective, because the whole length must be computed. I believe all
 well-behaved containers in python can be directly used in the condition to
 mean non-emptiness.

 I'm little bit worried about the case when there's no update and no reader
 either. It seems it'll get to the `SYNCHRONIZING` state. But as there's no
 reader, no reader will ever move from the old readers, so nothing will
 trigger the check for the emptiness of that set. It seems like the manager
 will get stuck in that state forever.
 {{{#!python
     def complete_update(self):
         """This method should be called when memmgr is notified by the
         builder of the completion of segment update. It changes the
         state from UPDATING to SYNCHRONIZING, and COPYING to READY. In
         the former case, set of reader modules that are using the
         "current" reader version of the segment are moved to the set
         that are using an "old" version of segment. If there are no such
         readers using the "old" version of segment, it pops the head
         (oldest) event from the pending events queue and returns it. It
         is an error if this method is called in other states than
         UPDATING and COPYING."""
         if self.__state == self.UPDATING:
             self.__state = self.SYNCHRONIZING
             self.__old_readers.update(self.__readers)
             self.__readers.clear()
             return self.__sync_reader_helper(self.SYNCHRONIZING)
         elif self.__state == self.COPYING:
             self.__state = self.READY
             return None
         else:
             raise SegmentInfoError('complete_update() called in ' +
                                    'incorrect state: ' +
 str(self.__state))
 }}}

 Why is the switching of `__readers` and `__old_readers` so complicated?
 Won't this be simpler and also faster?
 {{{#!python
 self.__old_readers = self.__readers
 self.__readers = set()
 }}}

 Why can't `remove_reader` happen any time? I don't see a reason why the
 auth server couldn't terminate or crash at any time it finds appropriate.
 Will the manager be required to buffer the unsubscriptions?

 The `sync_reader` and `remove_reader` seem to be quite similar. Could they
 be somewhat unified or share some of their code?

 Looking at this, it makes me wonder if it makes sense to test anything at
 all on the memory manager if the shared memory is not available. Would it
 make sense to completely exclude the memory manager from build then?
 {{{#!python
     @unittest.skipIf(os.environ['HAVE_SHARED_MEMORY'] != 'yes',
                      'shared memory is not available')
 }}}

 Would it be better to `assertEqual` for 1, or even better, for concrete
 value?
 {{{#!python
         self.assertNotEqual(len(self.__sgmt_info.get_events()), 0)
 }}}

 Why does it use `set()` sometimes and `{…}` sometimes? Why do we test that
 python doesn't care about order of values in set?
 {{{#!python
     def test_add_reader(self):
         self.assertSetEqual(self.__sgmt_info.get_readers(), set())
         self.assertSetEqual(self.__sgmt_info.get_old_readers(), set())
         self.__sgmt_info.add_reader(1)

         self.assertSetEqual(self.__sgmt_info.get_readers(), {1})
         self.__sgmt_info.add_reader(3)
         self.assertSetEqual(self.__sgmt_info.get_readers(), {1, 3})
         # ordering doesn't matter in sets
         self.__sgmt_info.add_reader(2)
         self.assertSetEqual(self.__sgmt_info.get_readers(), {1, 2, 3})
         self.assertSetEqual(self.__sgmt_info.get_readers(), {1, 3, 2})
 }}}

 Is it really allowed to call `start_update` any time?

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