-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Refactor getting replication updates from database. #7636
Changes from 3 commits
cc34308
8de532d
53db1be
1975a4d
85c9a94
67e7276
b6e35f2
9b492b6
d00dce6
d3ed450
dbe1760
df6c3b0
a902de2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1077,9 +1077,14 @@ def get_ex_outlier_stream_rows_txn(txn): | |
"get_ex_outlier_stream_rows", get_ex_outlier_stream_rows_txn | ||
) | ||
|
||
def get_all_new_backfill_event_rows(self, last_id, current_id, limit): | ||
async def get_all_new_backfill_event_rows( | ||
self, instance_name: str, last_id: int, current_id: int, limit: int | ||
) -> Tuple[List[Tuple[int, list]], int, bool]: | ||
"""Get updates for backfill replication stream, including all new | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you document the args and return format of the update functions? (Are the ids inclusive or exclusive?) I know they are all much the same, but if I'm working on the storage layer, I don't want to have to go digging into how the replication layer works to know what a given function is supposed to do, and not having it written down explicitly is a good way for assumptions to be made and off-by-one errors to get in. |
||
if last_id == current_id: | ||
return defer.succeed([]) | ||
return [], current_id, False | ||
|
||
def get_all_new_backfill_event_rows(txn): | ||
sql = ( | ||
|
@@ -1094,10 +1099,12 @@ def get_all_new_backfill_event_rows(txn): | |
" LIMIT ?" | ||
) | ||
txn.execute(sql, (-last_id, -current_id, limit)) | ||
new_event_updates = txn.fetchall() | ||
new_event_updates = [(row[0], row[1:]) for row in txn] | ||
|
||
limited = False | ||
if len(new_event_updates) == limit: | ||
upper_bound = new_event_updates[-1][0] | ||
limited = True | ||
else: | ||
upper_bound = current_id | ||
|
||
|
@@ -1114,11 +1121,15 @@ def get_all_new_backfill_event_rows(txn): | |
" ORDER BY event_stream_ordering DESC" | ||
) | ||
txn.execute(sql, (-last_id, -upper_bound)) | ||
new_event_updates.extend(txn.fetchall()) | ||
new_event_updates.extend((row[0], row[1:]) for row in txn) | ||
|
||
if len(new_event_updates) >= limit: | ||
upper_bound = new_event_updates[-1][0] | ||
limited = True | ||
|
||
return new_event_updates | ||
return new_event_updates, upper_bound, limited | ||
|
||
return self.db.runInteraction( | ||
return await self.db.runInteraction( | ||
"get_all_new_backfill_event_rows", get_all_new_backfill_event_rows | ||
) | ||
|
||
|
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.
doesn't this need more intelligence?
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.
Woops, for some reason i thought we weren't limiting for this