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

Use stream cache in get_linearized_receipts_for_room #3505

Merged
merged 4 commits into from
Jul 12, 2018

Conversation

erikjohnston
Copy link
Member

This avoids us from uncessarily hitting the database when there has been
no change for the room

This avoids us from uncessarily hitting the database when there has been
no change for the room
@erikjohnston erikjohnston requested a review from a team July 10, 2018 16:26
@@ -164,6 +163,16 @@ def get_linearized_receipts_for_room(self, room_id, to_key, from_key=None):
Returns:
list: A list of receipts.
"""
if from_key:
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be if from_key is not None?

@@ -164,6 +163,16 @@ def get_linearized_receipts_for_room(self, room_id, to_key, from_key=None):
Returns:
list: A list of receipts.
"""
if from_key:
if not self._receipts_stream_cache.has_entity_changed(room_id, from_key):
Copy link
Member

Choose a reason for hiding this comment

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

can we have a comment here please? why is this a sensible thing to do?

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 modulo docstring nit

@@ -164,6 +165,18 @@ def get_linearized_receipts_for_room(self, room_id, to_key, from_key=None):
Returns:
list: A list of receipts.
Copy link
Member

Choose a reason for hiding this comment

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

could you clarify that this is a Deferred[list] ?

@richvdh richvdh assigned erikjohnston and unassigned richvdh Jul 12, 2018
@erikjohnston erikjohnston merged commit 0456e05 into develop Jul 12, 2018
@erikjohnston erikjohnston deleted the erikj/receipts_cahce branch September 20, 2018 13:59
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