Use StateResolutionHandler to resolve state in persist_events #2864
Conversation
rejig the if statements to simplify the logic and reduce indentation
... and thus benefit (hopefully) from its cache.
synapse/storage/events.py
Outdated
# I don't think this can happen, but let's double-check | ||
raise Exception( | ||
"Context for new extremity event %s has no state " | ||
"group" % event_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I do prefer always using tuples rather than relying on strings working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in you'd prefer (event_id, )
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally yes, (or (event_id,)
without the space), as its very confusing if there is a bug and event_id
suddenly becomes iterable and then the logging explodes. In this particular case its probably fine though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair
synapse/storage/events.py
Outdated
@@ -586,6 +586,7 @@ def get_events(ev_ids): | |||
|
|||
current_state = yield resolve_events_with_factory( | |||
state_sets, | |||
event_map={}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be None to be a bit more consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plausibly, but it's replaced in ebfe64e anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, didn't realise until after i had submitted the review
... and thus benefit (hopefully) from its cache.