Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Split out DB writes in federation handler #3621

Merged
merged 5 commits into from
Aug 2, 2018

Conversation

erikjohnston
Copy link
Member

This will allow us to easily add an internal replication API to proxy
these reqeusts to master, so that we can move federation APIs to
workers.

This will allow us to easily add an internal replication API to proxy
these reqeusts to master, so that we can move federation APIs to
workers.
@erikjohnston erikjohnston requested a review from a team July 27, 2018 13:52
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some nits

which might update the current state etc.

Returns:
Deferred[int]: he stream ordering of the latest persisted event
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he

)

defer.returnValue((event_stream_id, max_stream_id))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update the docstring?

@@ -1616,12 +1551,10 @@ def _persist_auth_tree(self, origin, auth_events, state, event):
event, old_state=state
)

event_stream_id, max_stream_id = yield self.store.persist_event(
event, new_event_context,
yield self._persist_events(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to keep track of which methods do the notification and which do not. Could you update the docstring on persist_auth_tree?

@@ -1517,7 +1452,7 @@ def _handle_new_events(self, origin, event_infos, backfilled=False):
], consumeErrors=True,
))

yield self.store.persist_events(
yield self._persist_events(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, can you update the docstring to say that we may notify

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Aug 1, 2018
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@richvdh richvdh assigned erikjohnston and unassigned richvdh Aug 1, 2018
@erikjohnston erikjohnston merged commit 40c1c59 into develop Aug 2, 2018
@erikjohnston erikjohnston deleted the erikj/split_fed_store branch September 20, 2018 13:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants