-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Refactor backfilled
into specific behavior function arguments pt.1
#11396
Refactor backfilled
into specific behavior function arguments pt.1
#11396
Conversation
35e036a
to
96d6b5c
Compare
backfilled
behavior into specific behavior function parametersbackfilled
behavior into specific behavior function arguments
96d6b5c
to
f5e3731
Compare
f5e3731
to
d203d22
Compare
backfilled
behavior into specific behavior function argumentsbackfilled
behavior into specific behavior function arguments pt.1
backfilled
behavior into specific behavior function arguments pt.1backfilled
arguments into specific behavior function arguments pt.1
backfilled
arguments into specific behavior function arguments pt.1backfilled
into specific behavior function arguments pt.1
``` synapse/replication/http/federation.py:72: error: Signature of "_serialize_payload" incompatible with supertype "ReplicationEndpoint" [override] ```
should_calculate_state_and_forward_extrems, | ||
use_negative_stream_ordering, | ||
inhibit_local_membership_updates, | ||
update_room_forward_stream_ordering, |
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.
Is there a way we can define the arguments here like the following? I can't get it to work even without the keyword-only marker.
async def _serialize_payload(
store,
room_id,
event_and_contexts,
*,
inhibit_push_notifications: bool = False,
should_calculate_state_and_forward_extrems: bool = True,
use_negative_stream_ordering: bool = False,
inhibit_local_membership_updates: bool = False,
update_room_forward_stream_ordering: bool = True,
):
When I do this, I get the following error:
synapse/replication/http/federation.py:72: error: Signature of "_serialize_payload" incompatible with supertype "ReplicationEndpoint" [override]
And I can't figure out how to adjust the abstract method to be more flexible,
synapse/synapse/replication/http/_base.py
Lines 131 to 132 in 7ffddd8
@abc.abstractmethod | |
async def _serialize_payload(**kwargs): |
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.
I'm not entirely sure why, but it's the :bool
type annotations that are upsetting it rather than the *
or the default values. If I remove the :bool
s, it's fine.
@@ -35,6 +35,7 @@ | |||
) | |||
|
|||
import attr | |||
from mypy_extensions import DefaultNamedArg |
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.
Is there a better way to tackle this?
The build is failing because I used it builtins.ModuleNotFoundError: No module named 'mypy_extensions'
, https://github.com/matrix-org/synapse/runs/4261552290?check_suite_focus=true#step:6:739
backfilled
into specific behavior function arguments pt.1backfilled
into specific behavior function arguments pt.1
# Backfilled events have negative stream_ordering and happened | ||
# in the past so we know that we don't need to update the | ||
# stream_ordering tip for the room. | ||
update_room_forward_stream_ordering=not backfilled, |
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.
In a future PR, I'll extend the behavior arguments further upstream in place of backfilled
.
This PR does not replace all backfilled
instances. Just persist_events_and_notify
and all downstream calls
) | ||
|
||
if self._ephemeral_messages_enabled: | ||
for event in events: | ||
# If there's an expiry timestamp on the event, schedule its expiry. | ||
self._message_handler.maybe_schedule_expiry(event) | ||
|
||
if not backfilled: # Never notify for backfilled events | ||
if not inhibit_push_notifications: # Never notify for backfilled events |
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.
Behavioral argument replacement: inhibit_push_notifications
@@ -448,7 +590,7 @@ async def _persist_event_batch( | |||
# device lists as stale. | |||
potentially_left_users: Set[str] = set() | |||
|
|||
if not backfilled: | |||
if should_calculate_state_and_forward_extrems: |
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.
Behavioral argument replacement: should_calculate_state_and_forward_extrems
@@ -159,7 +175,7 @@ async def _persist_events_and_state_updates( | |||
# | |||
# Note: Multiple instances of this function cannot be in flight at | |||
# the same time for the same room. | |||
if backfilled: | |||
if use_negative_stream_ordering: |
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.
Behavior argument replacement: use_negative_stream_ordering
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.
Behavior argument replacement:
use_negative_stream_ordering
sorry, what does this mean?
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.
It's just the root place that I originally replaced and defines the behavior of the new argument.
@@ -1682,7 +1740,7 @@ def non_null_str_or_none(val: Any) -> Optional[str]: | |||
# band membership", like a remote invite or a rejection of a remote invite. | |||
if ( | |||
self.is_mine_id(event.state_key) | |||
and not backfilled | |||
and not inhibit_local_membership_updates |
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.
Behavioral argument replacement: inhibit_local_membership_updates
state_delta_for_room=state_delta_for_room, | ||
new_forward_extremeties=new_forward_extremeties, | ||
) | ||
persist_event_counter.inc(len(events_and_contexts)) | ||
|
||
if not backfilled: | ||
# TODO: test that this actuall works | ||
if stream < 0: |
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.
TODO: Test that this actually works
""" | ||
depth_updates: Dict[str, int] = {} | ||
for event, context in events_and_contexts: | ||
# Remove the any existing cache entries for the event_ids | ||
txn.call_after(self.store._invalidate_get_event_cache, event.event_id) | ||
if not backfilled: | ||
if update_room_forward_stream_ordering: |
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.
Behavioral argument replacement: update_room_forward_stream_ordering
should_calculate_state_and_forward_extrems: bool = True, | ||
use_negative_stream_ordering: bool = False, | ||
inhibit_local_membership_updates: bool = False, | ||
update_room_forward_stream_ordering: bool = True, |
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.
@richvdh How does this look? Is this on the right track?
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.
Thanks for putting this up, @MadLittleMods!
I haven't read all the way through the diff, because it's a bit hard to follow. Could I ask you to break it down a bit - let's start somewhere nearer the bottom of the stack: say _persist_events_and_state_updates
. I think that will make it much easier to convince ourselves that the changes are correct.
Generally it seems like the right sort of thing, but I fear we might be taking it to extremes. If there are ways of grouping some of these flags together into coherent bits of functionality, I think that would be better. For example - does it ever make sense to set use_negative_stream_ordering=True
and update_room_forward_stream_ordering=True
together (or both False
)? If not, let's get rid of update_room_forward_stream_ordering
- otherwise it's just a nightmare to use the function.
should_calculate_state_and_forward_extrems, | ||
use_negative_stream_ordering, | ||
inhibit_local_membership_updates, | ||
update_room_forward_stream_ordering, |
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.
I'm not entirely sure why, but it's the :bool
type annotations that are upsetting it rather than the *
or the default values. If I remove the :bool
s, it's fine.
# Backfilled events have negative stream_ordering and happened | ||
# in the past so we know that we don't need to update the | ||
# stream_ordering tip for the room. | ||
update_room_forward_stream_ordering=not backfilled, |
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.
shouldn't update_room_forward_stream_ordering
be implied by use_negative_stream_ordering
?
# come down incremental `/sync` | ||
use_negative_stream_ordering=backfilled, | ||
# Backfilled events do not affect the current local state | ||
inhibit_local_membership_updates=backfilled, |
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.
would it be sensible to have this implied by should_calculate_state_and_forward_extrems
?
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.
I understand the mental leap where updating the state in the room also encompasses member state events.
My first impression was that it wasn't very obvious but it makes sense ⏩
# there is no need to update the forward extrems because we | ||
# already know this event happened in the past if it was | ||
# backfilled. | ||
should_calculate_state_and_forward_extrems=not backfilled, |
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.
the should_
prefix seems a bit redundant here.
@@ -1850,7 +1895,7 @@ async def persist_events_and_notify( | |||
store=self._store, | |||
room_id=room_id, | |||
event_and_contexts=batch, | |||
backfilled=backfilled, | |||
inhibit_push_notifications=inhibit_push_notifications, |
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.
doesn't this need to pass through all the keyword args?
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.
Good catch 👍 I think so. Should match _serialize_payload
.
It's a loose connection and I was having trouble typing it correctly, #11396 (comment), to have the linter yell at me for this.
@@ -159,7 +175,7 @@ async def _persist_events_and_state_updates( | |||
# | |||
# Note: Multiple instances of this function cannot be in flight at | |||
# the same time for the same room. | |||
if backfilled: | |||
if use_negative_stream_ordering: |
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.
Behavior argument replacement:
use_negative_stream_ordering
sorry, what does this mean?
Refactor
backfilled
into specific behavior function arguments pt.1. This PR does not replace allbackfilled
instances. Justpersist_events_and_notify
and all downstream callsUsing keyword-only arguments (
*
) to reduce the mistakes frombackfilled
being left as a positional argument somewhere and being interpreted wrong by our new arguments.Part of #11300
Dev notes
persist_events_and_notify
(addedinhibit_push_notifications
)persist_events
andpersist_event
_event_persist_queue
_event_persist_queue
->_persist_event_batch
(addedshould_calculate_state_and_forward_extrems
)_persist_events_and_state_updates
(addeduse_negative_stream_ordering
)_persist_events_txn
_update_room_depths_txn
(addedupdate_room_forward_stream_ordering
)_update_metadata_tables_txn
_store_room_members_txn
(addedinhibit_local_membership_updates
)Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.Pull request includes a sign off(run the linters)