BIND 10 #2856: memory manager initialization
BIND 10 Development
do-not-reply at isc.org
Mon Jul 22 10:22:26 UTC 2013
#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
Replying to [comment:12 muks]:
> > 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.
These methods:
* complete_update()
* sync_reader()
* start_update()
* remove_reader()
Whenever they figure out a state change is needed, they return it as their
result and pop from the queue. Or they return None.
The caller then needs to check the return value of each of these methods,
but probably handle it the same way. It doesn't sound like convenient API
of the class.
I know it is described in the ticket this way, though.
> > 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.
collections.deque
> > 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?
Skip the SYNCHRONIZING state directly?
> > 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?
No, it overwrites. But, at the moment when it happens, isn't the target
empty?
> > 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.
I think we should allow it.
> > 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.
Well, we didn't manage to get rid of local segments yet. But the
usefulness of memmgr without the shared memory is just completely zero,
because the local segments are just local in there.
> > 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?
I don't know. I don't have completely clear and detailed view of how the
rest will look like, but I think the operation makes no sense in many
contexts, so we probably should disallow it then.
> Did all the documentation look OK to you?
I think it looked OK.
Also, the tests fail for me:
{{{
EE.E
======================================================================
ERROR: test_configure (__main__.TestMemmgr)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 120, in test_configure
parse_answer(self.__mgr._config_handler({})))
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 76, in _config_handler
logger.debug(logger.DBGLVL_TRACE_BASIC, MEMMGR_CONFIG_UPDATE)
NameError: global name 'MEMMGR_CONFIG_UPDATE' is not defined
======================================================================
ERROR: test_datasrc_config_handler (__main__.TestMemmgr)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 178, in test_datasrc_config_handler
self.__mgr._datasrc_config_handler({}, cfg_data)
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 208, in _datasrc_config_handler
logger.info(MEMMGR_DATASRC_RECONFIGURED, genid)
NameError: global name 'MEMMGR_DATASRC_RECONFIGURED' is not defined
======================================================================
ERROR: test_setup_module (__main__.TestMemmgr)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 181, in _setup_module
'data_sources', self._datasrc_config_handler)
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 41, in add_remote_config_by_name
raise self.add_remote_exception
isc.config.ccsession.ModuleCCSessionError: faked exception
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 163, in test_setup_module
self.__mgr._setup_module)
File "/usr/lib64/python3.3/unittest/case.py", line 570, in assertRaises
return context.handle('assertRaises', callableObj, args, kwargs)
File "/usr/lib64/python3.3/unittest/case.py", line 135, in handle
callable_obj(*args, **kwargs)
File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 183, in _setup_module
logger.error(MEMMGR_NO_DATASRC_CONF, ex)
NameError: global name 'MEMMGR_NO_DATASRC_CONF' is not defined
----------------------------------------------------------------------
}}}
And, the copyright year in `isc/memmgr/logger.py` is wrong.
--
Ticket URL: <http://bind10.isc.org/ticket/2856#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list