Add concept of StatelessContext #3550
Conversation
This allows us to lazily load state on the master process as required (when using workers). The advantage of this is that in the common case the master doesn't need the full state of the events to persist them. We keep the concept of an EventContext that has the current_state_ids and prev_state_ids attributes, to avoid having to rewrite a lot of code. Instead, we shift the necessary functions to accept StatelessContext.
synapse/events/snapshot.py
Outdated
| prev_group (int): Previously persisted state group. ``None`` for an | ||
| outlier. | ||
| delta_ids (dict[(str, str), str]): Delta from ``prev_group``. | ||
| (type, state_key) -> event_id. ``None`` for an outlier. | ||
|
|
||
| prev_state_events (?): XXX: is this ever set to anything other than | ||
| the empty list? | ||
|
|
||
| current_state_ids (dict[(str, str), str]|None): | ||
| The current state map including the current event. |
There was a problem hiding this comment.
could we record the circumstances under which we expect it to be set, and which None? (likewise prev_state_ids)
synapse/events/snapshot.py
Outdated
| """Gets the current state IDs | ||
|
|
||
| Returns: | ||
| Deferred[dict[(str, str), str]|None] |
There was a problem hiding this comment.
do we expect it to return None, ever?
synapse/events/snapshot.py
Outdated
| @@ -130,24 +190,56 @@ def deserialize(store, input): | |||
|
|
|||
| # We use the state_group and prev_state_id stuff to pull the | |||
| # current_state_ids out of the DB and construct prev_state_ids. | |||
There was a problem hiding this comment.
this comment seems a bit out-of-place now
synapse/events/snapshot.py
Outdated
| """Called to populate the current_state_ids and prev_state_ids | ||
| attributes by loading from the database. | ||
| """ | ||
| self._have_fetched_state = True |
There was a problem hiding this comment.
this looks racy: what if two threads call get_current_state_ids at once?
There was a problem hiding this comment.
[I suggest you store a deferred while the fetch is taking place]
synapse/events/snapshot.py
Outdated
| self.current_state_ids = yield store.get_state_ids_for_group( | ||
| self.state_group, | ||
| ) | ||
| if self._prev_state_id: |
There was a problem hiding this comment.
shouldn't this be conditional on event_state_key being not-None?
synapse/events/snapshot.py
Outdated
| from frozendict import frozendict | ||
|
|
||
| from twisted.internet import defer | ||
|
|
||
|
|
||
| class EventContext(object): | ||
| class StatelessContext(object): |
There was a problem hiding this comment.
this feels like a funny name, given it may have a state.
Indeed I wonder if having it separate from EventContext is worthwhile? maybe combine them and have DeserializedContext just override the accessors?
There was a problem hiding this comment.
Unfortunately we can't just override accessors since they would need to return a deferreds in the DeserializedContext
There was a problem hiding this comment.
yes, but that's also true for EventContext. I don't understand why that means we can't override the accessors.
erikjohnston
force-pushed
the
erikj/stateless_contexts_3
branch
from
b3290f2
to
7350875
Compare
synapse/events/snapshot.py
Outdated
| context.prev_state_ids[(event_type, event_state_key)] = prev_state_id | ||
| else: | ||
| context.prev_state_ids = context.current_state_ids | ||
| context._have_fetched_state = None |
There was a problem hiding this comment.
if this is going to be a Deferred, then it needs a different name.
synapse/events/snapshot.py
Outdated
| """Implements StatelessContext""" | ||
|
|
||
| if not self._have_fetched_state: | ||
| self._have_fetched_state = self._fill_out_state(store) |
There was a problem hiding this comment.
you need a run_in_background here (and in get_prev_state_ids)
erikjohnston
force-pushed
the
erikj/stateless_contexts_3
branch
from
5ee3ba3
to
fecd7c2
Compare
There was a problem hiding this comment.
lgtm except for #3550 (comment) which I still don't understan
This allows us to lazily load state on the master process as required
(when using workers). The advantage of this is that in the common case
the master doesn't need the full state of the events to persist them.
We keep the concept of an EventContext that has the current_state_ids
and prev_state_ids attributes, to avoid having to rewrite a lot of code.
Instead, we shift the necessary functions to accept StatelessContext.
Supersedes #2999
I don't know whether it would be better to completely remove
current_state_idsfromStatelessContextor not.