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

Remove duplicate call to wake a remote destination when using federation sending worker #16515

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Remove duplicate call to wake a remote destination when using federation sending worker #16515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
618a833
remote extraneous call that duplicates the function below it
realtyem Oct 16, 2023
1d1044c
[MAYBE REVERT] These appear to not be used anywhere now
realtyem Oct 16, 2023
fd675f6
changelog
realtyem Oct 18, 2023
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/16515.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove duplicate call to mark remote server 'awake' when using a federation sending worker.
11 changes: 0 additions & 11 deletions synapse/replication/tcp/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,14 +279,6 @@ async def on_position(
# may be streaming.
self.notifier.notify_replication()

def on_remote_server_up(self, server: str) -> None:
"""Called when get a new REMOTE_SERVER_UP command."""

# Let's wake up the transaction queue for the server in case we have
# pending stuff to send to it.
if self.send_handler:
self.send_handler.wake_destination(server)

async def wait_for_stream_position(
self,
instance_name: str,
Expand Down Expand Up @@ -405,9 +397,6 @@ def __init__(self, hs: "HomeServer"):

self._fed_position_linearizer = Linearizer(name="_fed_position_linearizer")

def wake_destination(self, server: str) -> None:
self.federation_sender.wake_destination(server)

async def process_replication_rows(
self, stream_name: str, token: int, rows: list
) -> None:
Expand Down
2 changes: 0 additions & 2 deletions synapse/replication/tcp/handler.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I think your analysis is correct; I did a quick flowchart of all the calls from these:

flowchart
    ReplicationCommandHandler.on_REMOTE_SERVER_UP --> ReplicationDataHandler.on_remote_server_up
    ReplicationDataHandler.on_remote_server_up -->|If shouldSendFederation| FederationSenderHandler.wake_destination
    FederationSenderHandler.wake_destination --> FederationSender.wake_destination

    ReplicationCommandHandler.on_REMOTE_SERVER_UP --> Notifier.notify_remote_server_up
    Notifier.notify_remote_server_up -->|If shouldSendFederation| AbstractFederationSender.wake_destination
    AbstractFederationSender.wake_destination -.->|shouldSendFederation guarantees this path| FederationSender.wake_destination
    FederationSender.wake_destination --> PerDestinationQueue.attempt_new_transaction

    AbstractFederationSender.wake_destination -.->FederationRemoteSendQueue.wake_destination

    Notifier.notify_remote_server_up --> MatrixFederationHttpClient.wake_destination
    MatrixFederationHttpClient.wake_destination --> AwakenableSleeper.wake
Loading

The tl;dr is that it looks like we can remove the ReplicationDataHandler path, as you did.

It looks like previously the notifier was only called on the main process, but #7352 changed this once the remote server up command was not relayed via the main process.

Original file line number Diff line number Diff line change
Expand Up @@ -657,8 +657,6 @@ def on_REMOTE_SERVER_UP(
self, conn: IReplicationConnection, cmd: RemoteServerUpCommand
) -> None:
"""Called when get a new REMOTE_SERVER_UP command."""
self._replication_data_handler.on_remote_server_up(cmd.data)

self._notifier.notify_remote_server_up(cmd.data)

def on_LOCK_RELEASED(
Expand Down