From 0d1d3e070886694eff1fa862cd203206b1a63372 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 15 Jun 2022 16:17:14 +0100 Subject: Speed up `get_unread_event_push_actions_by_room` (#13005) Fixes #11887 hopefully. The core change here is that `event_push_summary` now holds a summary of counts up until a much more recent point, meaning that the range of rows we need to count in `event_push_actions` is much smaller. This needs two major changes: 1. When we get a receipt we need to recalculate `event_push_summary` rather than just delete it 2. The logic for deleting `event_push_actions` is now divorced from calculating `event_push_summary`. In future it would be good to calculate `event_push_summary` while we persist a new event (it should just be a case of adding one to the relevant rows in `event_push_summary`), as that will further simplify the get counts logic and remove the need for us to periodically update `event_push_summary` in a background job. --- tests/push/test_http.py | 16 ++++++++-------- tests/replication/slave/storage/test_events.py | 23 +++++++++++++++++------ tests/storage/test_event_push_actions.py | 24 +++++++++++++++++++++--- 3 files changed, 46 insertions(+), 17 deletions(-) (limited to 'tests') diff --git a/tests/push/test_http.py b/tests/push/test_http.py index ba158f5d93..d9c68cdd2d 100644 --- a/tests/push/test_http.py +++ b/tests/push/test_http.py @@ -577,7 +577,7 @@ class HTTPPusherTests(HomeserverTestCase): # Carry out our option-value specific test # # This push should still only contain an unread count of 1 (for 1 unread room) - self._check_push_attempt(6, 1) + self._check_push_attempt(7, 1) @override_config({"push": {"group_unread_count_by_room": False}}) def test_push_unread_count_message_count(self) -> None: @@ -591,7 +591,7 @@ class HTTPPusherTests(HomeserverTestCase): # # We're counting every unread message, so there should now be 3 since the # last read receipt - self._check_push_attempt(6, 3) + self._check_push_attempt(7, 3) def _test_push_unread_count(self) -> None: """ @@ -641,18 +641,18 @@ class HTTPPusherTests(HomeserverTestCase): response = self.helper.send( room_id, body="Hello there!", tok=other_access_token ) - # To get an unread count, the user who is getting notified has to have a read - # position in the room. We'll set the read position to this event in a moment + first_message_event_id = response["event_id"] expected_push_attempts = 1 - self._check_push_attempt(expected_push_attempts, 0) + self._check_push_attempt(expected_push_attempts, 1) self._send_read_request(access_token, first_message_event_id, room_id) - # Unread count has not changed. Therefore, ensure that read request does not - # trigger a push notification. - self.assertEqual(len(self.push_attempts), 1) + # Unread count has changed. Therefore, ensure that read request triggers + # a push notification. + expected_push_attempts += 1 + self.assertEqual(len(self.push_attempts), expected_push_attempts) # Send another message response2 = self.helper.send( diff --git a/tests/replication/slave/storage/test_events.py b/tests/replication/slave/storage/test_events.py index 6d3d4afe52..531a0db2d0 100644 --- a/tests/replication/slave/storage/test_events.py +++ b/tests/replication/slave/storage/test_events.py @@ -15,7 +15,9 @@ import logging from typing import Iterable, Optional from canonicaljson import encode_canonical_json +from parameterized import parameterized +from synapse.api.constants import ReceiptTypes from synapse.api.room_versions import RoomVersions from synapse.events import FrozenEvent, _EventInternalMetadata, make_event_from_dict from synapse.handlers.room import RoomEventSource @@ -156,17 +158,26 @@ class SlavedEventStoreTestCase(BaseSlavedStoreTestCase): ], ) - def test_push_actions_for_user(self): + @parameterized.expand([(True,), (False,)]) + def test_push_actions_for_user(self, send_receipt: bool): self.persist(type="m.room.create", key="", creator=USER_ID) - self.persist(type="m.room.join", key=USER_ID, membership="join") + self.persist(type="m.room.member", key=USER_ID, membership="join") self.persist( - type="m.room.join", sender=USER_ID, key=USER_ID_2, membership="join" + type="m.room.member", sender=USER_ID, key=USER_ID_2, membership="join" ) event1 = self.persist(type="m.room.message", msgtype="m.text", body="hello") self.replicate() + + if send_receipt: + self.get_success( + self.master_store.insert_receipt( + ROOM_ID, ReceiptTypes.READ, USER_ID_2, [event1.event_id], {} + ) + ) + self.check( "get_unread_event_push_actions_by_room_for_user", - [ROOM_ID, USER_ID_2, event1.event_id], + [ROOM_ID, USER_ID_2], NotifCounts(highlight_count=0, unread_count=0, notify_count=0), ) @@ -179,7 +190,7 @@ class SlavedEventStoreTestCase(BaseSlavedStoreTestCase): self.replicate() self.check( "get_unread_event_push_actions_by_room_for_user", - [ROOM_ID, USER_ID_2, event1.event_id], + [ROOM_ID, USER_ID_2], NotifCounts(highlight_count=0, unread_count=0, notify_count=1), ) @@ -194,7 +205,7 @@ class SlavedEventStoreTestCase(BaseSlavedStoreTestCase): self.replicate() self.check( "get_unread_event_push_actions_by_room_for_user", - [ROOM_ID, USER_ID_2, event1.event_id], + [ROOM_ID, USER_ID_2], NotifCounts(highlight_count=1, unread_count=0, notify_count=2), ) diff --git a/tests/storage/test_event_push_actions.py b/tests/storage/test_event_push_actions.py index 0f9add4841..4273524c4c 100644 --- a/tests/storage/test_event_push_actions.py +++ b/tests/storage/test_event_push_actions.py @@ -51,10 +51,16 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): room_id = "!foo:example.com" user_id = "@user1235:example.com" + last_read_stream_ordering = [0] + def _assert_counts(noitf_count, highlight_count): counts = self.get_success( self.store.db_pool.runInteraction( - "", self.store._get_unread_counts_by_pos_txn, room_id, user_id, 0 + "", + self.store._get_unread_counts_by_pos_txn, + room_id, + user_id, + last_read_stream_ordering[0], ) ) self.assertEqual( @@ -98,6 +104,7 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): ) def _mark_read(stream, depth): + last_read_stream_ordering[0] = stream self.get_success( self.store.db_pool.runInteraction( "", @@ -144,8 +151,19 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): _assert_counts(1, 1) _rotate(9) _assert_counts(1, 1) - _rotate(10) - _assert_counts(1, 1) + + # Check that adding another notification and rotating after highlight + # works. + _inject_actions(10, PlAIN_NOTIF) + _rotate(11) + _assert_counts(2, 1) + + # Check that sending read receipts at different points results in the + # right counts. + _mark_read(8, 8) + _assert_counts(1, 0) + _mark_read(10, 10) + _assert_counts(0, 0) def test_find_first_stream_ordering_after_ts(self): def add_event(so, ts): -- cgit 1.4.1