BIND 10 #2856: memory manager initialization
BIND 10 Development
do-not-reply at isc.org
Fri Jul 19 12:12:36 UTC 2013
#2856: memory manager initialization
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | vorner
Priority: medium | Status:
Component: shmem | reviewing
manager | Milestone:
Keywords: | Sprint-20130723
Sensitive: 0 | Resolution:
Sub-Project: DNS | 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 muks):
* owner: muks => vorner
Comment:
Hi Michal
Replying to [comment:11 vorner]:
> 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.
There isn't anything in implementation connecting them. But I guess this
was included as ground work by Jinmei.
> 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.
I didn't follow the last sentence.
> 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'
> }}}
It didn't cleanup the mapped file after the test. The fixture has been
updated to do that now during teardown.
> 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
> }}}
This has been updated to log the error.
> 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.
Suggestion taken and logging has been added.
> Do we really start at `READY` in the `SegmentInfo` class? Won't we
> start by loading something and sending updates to the clients?
This is what the ticket explicitly specifies. I guess it's ok
considering we can start by `start_update()`.
> 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()
> }}}
If you mean the phrase "publicly readable", this means that there is a
getter, but there's no way to set it from outside the class.
> 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.
I am not able to find such a class. If you remember the name, please let
me know. The `queue.Queue` class is a producer-consumer queue.
> 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.
OK I have modified the code to test the variables directly for truth.
> 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))
> }}}
What do you suggest we do for it?
> 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()
> }}}
The current code performs a set union. I'm not sure if the assignment
does the same. Does it?
> 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?
Hmmm... shall we allow this then? I implemented the description to the
letter, but didn't consider the bigger picture in this particular
ticket.
> The `sync_reader` and `remove_reader` seem to be quite similar. Could
> they be somewhat unified or share some of their code?
It could be, but they differ in small ways. I think such unification
would make it harder to follow.
> 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')
> }}}
I think we still allow local segments to the memory manager. There is
another ticket about it, but this is something we can do afterwards.
> Would it be better to `assertEqual` for 1, or even better, for concrete
value?
> {{{#!python
> self.assertNotEqual(len(self.__sgmt_info.get_events()), 0)
> }}}
This is done too.
> 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 syntax for an empty `dict`.
> Is it really allowed to call `start_update` any time?
I thought it was weird too, but thought maybe I misunderstood the
description. Shall I update it?
Did all the documentation look OK to you?
--
Ticket URL: <http://bind10.isc.org/ticket/2856#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list