-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bust _membership_stream_cache
cache when current state changes
#17732
Changes from 1 commit
2bd0e63
d327c62
64eb71f
dd87620
7c53716
bceb4e0
944d1e0
20ff239
f40e649
f5f0e36
7e328d7
ac06ddf
e52f5f0
bf695e6
2d20b71
484d045
6d29960
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 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -219,6 +219,8 @@ def process_replication_rows( | |||||||||||||||||
room_id = row.keys[0] | ||||||||||||||||||
members_changed = set(row.keys[1:]) | ||||||||||||||||||
self._invalidate_state_caches(room_id, members_changed) | ||||||||||||||||||
for user_id in members_changed: | ||||||||||||||||||
self._membership_stream_cache.entity_has_changed(user_id, token) # type: ignore[attr-defined] | ||||||||||||||||||
MadLittleMods marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
elif row.cache_func == PURGE_HISTORY_CACHE_NAME: | ||||||||||||||||||
if row.keys is None: | ||||||||||||||||||
raise Exception( | ||||||||||||||||||
|
@@ -236,6 +238,7 @@ def process_replication_rows( | |||||||||||||||||
room_id = row.keys[0] | ||||||||||||||||||
self._invalidate_caches_for_room_events(room_id) | ||||||||||||||||||
self._invalidate_caches_for_room(room_id) | ||||||||||||||||||
Comment on lines
242
to
243
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. Separate from this PR and in line with the fact that these code paths need to be cleaned up, there is an obvious duplication here because we call |
||||||||||||||||||
self._membership_stream_cache.all_entities_changed(token) # type: ignore[attr-defined] | ||||||||||||||||||
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. We don't actually insert anything into Given how frequently rooms are deleted nowadays (as we delete rooms where everyone has left after N days, at least on matrix.org) I'm minded to remove these and leave a note. Otherwise I'm worried how this will affect perf 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. Using the same logic, why do we call Perhaps we consider the caches which only affect the 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.
We do delete a lot of stuff when deleting a room (obviously), and so those caches do contain different data than what is in the DB. So invalidation makes sense in that case? For the 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. So you're saying that we don't delete from That looks correct: synapse/synapse/storage/databases/main/purge_events.py Lines 502 to 508 in 2d23250
But
Overall, it feels very brittle to comment out And given we're only busting the 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. The issue is that
Note that the stream change caches effectively only cache the absence of changes, i.e. "nothing has changed between tokens X and Y and so don't query the database". Hence, I think, if we remove rows from the DB we don't need to clear the cache as at worst it will report that something has changed at token X, at which point it will query the DB and discover nothing is there? 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. I've commented out Since the |
||||||||||||||||||
else: | ||||||||||||||||||
self._attempt_to_invalidate_cache(row.cache_func, row.keys) | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -275,6 +278,7 @@ def _process_event_stream_row(self, token: int, row: EventsStreamRow) -> None: | |||||||||||||||||
self._attempt_to_invalidate_cache( | ||||||||||||||||||
"get_sliding_sync_rooms_for_user", None | ||||||||||||||||||
) | ||||||||||||||||||
self._membership_stream_cache.entity_has_changed(data.state_key, token) # type: ignore[attr-defined] | ||||||||||||||||||
elif data.type == EventTypes.RoomEncryption: | ||||||||||||||||||
self._attempt_to_invalidate_cache( | ||||||||||||||||||
"get_room_encryption", (data.room_id,) | ||||||||||||||||||
|
@@ -291,6 +295,7 @@ def _process_event_stream_row(self, token: int, row: EventsStreamRow) -> None: | |||||||||||||||||
# Similar to the above, but the entire caches are invalidated. This is | ||||||||||||||||||
# unfortunate for the membership caches, but should recover quickly. | ||||||||||||||||||
self._curr_state_delta_stream_cache.entity_has_changed(data.room_id, token) # type: ignore[attr-defined] | ||||||||||||||||||
self._membership_stream_cache.all_entities_changed(token) # type: ignore[attr-defined] | ||||||||||||||||||
self._attempt_to_invalidate_cache("get_rooms_for_user", None) | ||||||||||||||||||
self._attempt_to_invalidate_cache("get_room_type", (data.room_id,)) | ||||||||||||||||||
self._attempt_to_invalidate_cache("get_room_encryption", (data.room_id,)) | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,3 +251,19 @@ def test_max_pos(self) -> None: | |
|
||
# Unknown entities will return None | ||
self.assertEqual(cache.get_max_pos_of_last_change("[email protected]"), None) | ||
|
||
def test_all_entities_changed(self) -> None: | ||
""" | ||
`StreamChangeCache.all_entities_changed(...)` will mark all entites as changed. | ||
""" | ||
cache = StreamChangeCache("#test", 1, max_size=10) | ||
|
||
cache.entity_has_changed("[email protected]", 2) | ||
cache.entity_has_changed("[email protected]", 3) | ||
cache.entity_has_changed("[email protected]", 4) | ||
|
||
cache.all_entities_changed(5) | ||
|
||
self.assertEqual(cache.get_max_pos_of_last_change("[email protected]"), 5) | ||
self.assertEqual(cache.get_max_pos_of_last_change("[email protected]"), 5) | ||
self.assertEqual(cache.get_max_pos_of_last_change("[email protected]"), 5) |
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.
Kinda weird to just stick this here (same with the others in
process_replication_rows
). Better way to organize this?