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

Attempt to annotate events_forward_extremities #11314

Merged
DMRobertson merged 5 commits into from
Nov 12, 2021
Merged

Attempt to annotate events_forward_extremities #11314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f57fb1
Make DataStore inherit from EventForwardExtremitiesStore before Cache…
Nov 11, 2021
ab01dad
Attempt to anntoate events_forward_extermities
Nov 11, 2021
2e88d0f
Changelog.misc
Nov 11, 2021
4144d36
Merge branch 'develop' into dmr/typing/storage/events_forward_extermi…
Nov 11, 2021
2fa2676
Merge branch 'develop' into dmr/typing/storage/events_forward_extermi…
Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11314.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add type hints to storage classes.
1 change: 0 additions & 1 deletion mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ exclude = (?x)
|synapse/storage/databases/main/event_federation.py
|synapse/storage/databases/main/event_push_actions.py
|synapse/storage/databases/main/events_bg_updates.py
|synapse/storage/databases/main/events_forward_extremities.py
|synapse/storage/databases/main/events_worker.py
|synapse/storage/databases/main/group_server.py
|synapse/storage/databases/main/media_repository.py
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/databases/main/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ class DataStore(
RelationsStore,
CensorEventsStore,
UIAuthStore,
EventForwardExtremitiesStore,
CacheInvalidationWorkerStore,
ServerMetricsStore,
EventForwardExtremitiesStore,
LockStore,
SessionStore,
):
Expand Down
21 changes: 15 additions & 6 deletions synapse/storage/databases/main/events_forward_extremities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,20 @@
# limitations under the License.

import logging
from typing import Dict, List
from typing import Any, Dict, List

from synapse.api.errors import SynapseError
from synapse.storage._base import SQLBaseStore
from synapse.storage.database import LoggingTransaction
from synapse.storage.databases.main import CacheInvalidationWorkerStore
from synapse.storage.databases.main.event_federation import EventFederationWorkerStore

logger = logging.getLogger(__name__)


class EventForwardExtremitiesStore(SQLBaseStore):
class EventForwardExtremitiesStore(
EventFederationWorkerStore,
CacheInvalidationWorkerStore,
):
Comment on lines +26 to +29
Copy link
Member

@clokep clokep Nov 12, 2021

Choose a reason for hiding this comment

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

Any concerns this could introduce bugs? The only way I could see that happening is if we're shadowing methods, but I really hope we're not doing that unless it is a worker vs. non-worker version.

Copy link
Contributor Author

@DMRobertson DMRobertson Nov 12, 2021

Choose a reason for hiding this comment

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

I was scared of doing this, but I'm scared of the storage classes as a whole.

This is a very reasonable question that I don't know how to answer easily.

EventFederationWorkerStore has the following methods. Those without notes aren't defined anywhere else.

  • get_auth_chain
  • get_auth_chain_ids
  • _get_auth_chain_ids_using_cover_index_txn
  • _get_auth_chain_ids_txn
  • get_auth_chain_difference
    • is also defined on StateResolutionStore and is defined on TestStateResolutionStore. I think these are wrappers rather than storage mixins. (?)
  • _get_auth_chain_difference_using_cover_index_txn
  • _get_auth_chain_difference_txn
  • get_oldest_event_ids_with_depth_in_room
  • get_insertion_event_backwards_extremities_in_room
  • get_max_depth_of
  • get_min_depth_of
  • get_prev_events_for_room
  • _get_prev_events_for_room_txn
  • _get_prev_events_for_room_txn
  • get_latest_event_ids_in_room
  • get_min_depth
  • _get_min_depth_interaction
  • get_forward_extremities_for_room_at_stream_ordering
  • _get_forward_extremeties_for_room
  • get_backfill_events
  • _get_backfill_events
  • get_missing_events
    • defined on federation client and transport, but not any other store
  • _get_missing_events
  • get_successor_events
  • _delete_old_forward_extrem_cache
  • insert_insertion_extremity
  • insert_received_event_to_staging
  • remove_received_event_from_staging
  • get_next_staged_event_id_for_room
  • get_next_staged_event_for_room
  • prune_staged_events_in_room
  • get_all_rooms_with_staged_incoming_events
  • _get_stats_for_federation_staging

So the only shadowing is by a wrapper rather than a mixin. I think this is safe? But I'm mainly relying on CI not to explode to be honest.

CacheInvalidationWorkerStore to follow.

Copy link
Contributor Author

@DMRobertson DMRobertson Nov 12, 2021

Choose a reason for hiding this comment

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

CacheInvalidationWorkerStore has the following methods. Those without notes aren't defined elsewhere.

  • get_all_updated_caches
  • process_replication_rows defined on
    • CacheInvalidationWorkerStore
    • DeviceInboxWorkerStore
    • EventsWorkerStore
    • PrescenceStore
    • ReceiptsWorkerStore
    • each of these ends with a call to the same method on the super()class, so I think this will traverse the whole MRO. I don't know how we can enforce that though.
  • _process_event_stream_row
  • _invalidate_caches_for_event
  • invalidate_cache_and_stream
  • _invalidate_cache_and_stream
  • _invalidate_all_cache_and_stream
  • _invalidate_state_caches_and_stream
  • _send_invalidation_to_replication
  • get_cache_stream_token_for_writer

Given that every process_replication_rows implementation calls the superclass will be safe provided that EventForwardExtremitiesStore's MRO is consistent with that of DataStore and any other top-level Stores. If the latter was not the case, we'd get a big TypeError at import time when trying to define the DataStore type.

Copy link
Member

@clokep clokep Nov 12, 2021

Choose a reason for hiding this comment

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

Thanks for the thorough review! 👍 I agree with your assessment!

Copy link
Contributor Author

@DMRobertson DMRobertson Nov 12, 2021

Choose a reason for hiding this comment

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

I suspect we might need to do this again if I keep pulling in superclasses like this. Maybe there's some way to automate this process?

async def delete_forward_extremities_for_room(self, room_id: str) -> int:
"""Delete any extra forward extremities for a room.

Expand All @@ -31,7 +36,7 @@ async def delete_forward_extremities_for_room(self, room_id: str) -> int:
Returns count deleted.
"""

def delete_forward_extremities_for_room_txn(txn):
def delete_forward_extremities_for_room_txn(txn: LoggingTransaction) -> int:
# First we need to get the event_id to not delete
sql = """
SELECT event_id FROM event_forward_extremities
Expand Down Expand Up @@ -82,10 +87,14 @@ def delete_forward_extremities_for_room_txn(txn):
delete_forward_extremities_for_room_txn,
)

async def get_forward_extremities_for_room(self, room_id: str) -> List[Dict]:
async def get_forward_extremities_for_room(
self, room_id: str
) -> List[Dict[str, Any]]:
"""Get list of forward extremities for a room."""

def get_forward_extremities_for_room_txn(txn):
def get_forward_extremities_for_room_txn(
txn: LoggingTransaction,
) -> List[Dict[str, Any]]:
sql = """
SELECT event_id, state_group, depth, received_ts
FROM event_forward_extremities
Expand Down