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

Refactor getting replication updates from database. #7636

Merged
merged 13 commits into from
Jun 16, 2020
Merged
2 changes: 1 addition & 1 deletion synapse/handlers/typing.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ async def get_all_typing_updates(
typing = self._room_typing[room_id]
rows.append((serial, (room_id, list(typing))))
rows.sort()
return rows[:limit], current_id, False
return rows[:limit], current_id, len(rows) > limit
Copy link
Member

Choose a reason for hiding this comment

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

this is doing the wrong thing for the returned token when the limit is hit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am such a crank 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

MAYBE THIS TIME I'VE fIXeD IT?!!!??!1?


def get_current_token(self):
return self._latest_room_serial
Expand Down
15 changes: 15 additions & 0 deletions synapse/storage/data_stores/main/events_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -1082,6 +1082,21 @@ async def get_all_new_backfill_event_rows(
) -> Tuple[List[Tuple[int, list]], int, bool]:
"""Get updates for backfill replication stream, including all new
Copy link
Member

Choose a reason for hiding this comment

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

this docstring is very helpful, but please can all the updated/new storage methods have one, not just this method?

backfilled events and events that have gone from being outliers to not.

Args:
instance_name: The writer we want to fetch updates from. Unused
here sincethere is only ever one writer.
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
last_id: The token to fetch updates from. Exclusive.
current_id: The token to fetch updates up to. Inclusive.
limit: The requested limit for the number of rows to return. The
function may return more or fewer rows.

Returns:
A tuple consisting of: the updates, the position of the rows
returned up to, and whether we returned fewer rows than exists
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 rephrase "the position of the rows returned up to"? it's somewhat unclear: inconsistent use of "position" and "token", inclusive or exclusive, etc.

"the last token included in the results", maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully I've clarified it. Unfortunately, technically, the returned token doesn't have to be the last token included in the results (since current_id could be greater due to a gap)

between the requested tokens due to the limit.

The updates are list of 2-tuples of stream ID and the row.
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
"""
if last_id == current_id:
return [], current_id, False
Expand Down