Re: [reSIProcate] upcoming changes to DUM's RegistrationPersistenceManager and ServerRegistration
Overall, the proposal looks sound to me. A few minor comments inline.
Also, I know that David Bryan has been doing some investigation into how
to modify these data structures so that they have hooks to support
storage of P2P information in registration records. You probably want to
talk with him to determine if there are some things you can do in the
refactoring to make such changes easier and/or possible.
Rohan Mahy wrote:
class BaseContactRecord {
NameAddr contact; // can contain callee caps
time_t regExpires;
time_t lastUpdated;
Tuple receivedFrom; // source transport, IP address, and port
UriList sipPath; // Value of SIP Path header from the Request
};
Does the NameAddr contain the GRUU information also? That is, is this
going to be an exact copy of what comes off the wire, or do we have to
tear it apart and resynthesize it? If it's the latter, then I think I
like the other approach -- of storing the actual capabilities as a
separate item -- better. (In fact, it might even be better in the former
case). I'm thinking of how I would go about implementing a search for
the correct contact based on caller prefs; it would be much faster if
the callee caps were already broken down and pre-digested in an
appropriate structure (e.g. a hash of capabilities).
class Aor {
Uri aor;
SimpleContactList contacts;
InstanceList instances;
UriList aliases;
};
I suppose that the "aliases" field here is used to remove aliases when
deregistration occurs, and nothing else, right?
2. ServerRegistration::dispatch first processes a registration by
first locking then adding, updating, or deleting records in the
"database", then if reject is called, we roll back the changes.
accept() and reject() unlock the database. This is problematic for a
number of reasons. This is a DoS bait if I ever saw it. Sending a lot
of illegitimate REGISTERs can lock the database and prevent the
LocationServer from accessing the locked records.
When I did this, I intended to eventually overhaul it to be more like a
two-phase commit, where the database could confirm the update before
effecting it, but the application would have the opportunity to enforce
policy before readers had access. It was a Known Ugly Hack just to get
things up and running. :)
- We don't want Registrations to impact the actual database unless
they are actually accepted
- We should lock the database as briefly as possible. It would be
great if we only needed write locks (we could do this if all the
operations on the database were atomic)*
It's completely unclear to me how you can make reads through STL
structures atomic. I think you'll probably still need AOR-level locking
here. Read/Write locks make much more sense than mutexes.
I propose refactoring ServerRegistration so we pass a list of
operations (memory owned by DUM) in the ServerRegistrationHandlers.
The application can inspect the operations and/or the original
request. accept() would cause the list of operations to be committed
to the database (quick lock/unlock), return the current list of
registered contacts (with and without instances), and free the list of
operations. reject() would free the memory.
That sounds like a good design to me.
We can use the same operations for updates from other cluster members
when we add high availability.
Yep. I like that.
/a