Fix limit logic for AccountDataStream #7384
Conversation
Make sure that the AccountDataStream presents complete updates, in the right order.
|
|
||
| room_rows = ( | ||
| (stream_id, (user_id, room_id, account_data_type)) | ||
| for stream_id, user_id, room_id, account_data_type in room_results |
There was a problem hiding this comment.
I know that technically this doesn't need a if stream_id <= token clause, but I think adding may make it less confusing why we're omitting it (or add a comment)
| return results | ||
| # we need to return a sorted list, so merge them together. | ||
| updates = list(heapq.merge(room_rows, global_rows)) | ||
| return updates, to_token, limited |
There was a problem hiding this comment.
Any reason not to move some of this handling into the store? I'm mainly thinking it might be more efficient that way, if for no other reason that we would only need one transaction rather than two.
There was a problem hiding this comment.
I dunno, maybe?
It feels like that would be a different approach to that taken by all the other stream sources: it would mean we'd have to mess about with limited inside the store, which normally we don't do.
Another way would be to follow the example of get_all_device_list_changes_for_remotes and do one query with a UNION, but I only found that after I wrote this stuff...
Make sure that the AccountDataStream presents complete updates, in the right order. This is much the same fix as matrix-org#7337 and matrix-org#7358, but applied to a different stream.
Make sure that the AccountDataStream presents complete updates, in the right
order.
This is much the same fix as #7337 and #7358, but applied to a different stream.