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

Handle replication commands synchronously where possible #7876

Merged
merged 13 commits into from
Jul 27, 2020

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jul 16, 2020

Most of the stuff we do for replication commands can be done synchronously. There's no point spinning up background processes if we're not going to need them.

@richvdh richvdh force-pushed the rav/synchronous_replication branch 3 times, most recently from a2999a7 to ad48936 Compare July 16, 2020 22:47
... and only start a background process if they are async.
fire up a background process only when we start processing the queue.
@richvdh richvdh force-pushed the rav/synchronous_replication branch from ad48936 to ebee62d Compare July 16, 2020 23:08
@richvdh richvdh requested a review from a team July 16, 2020 23:09
synapse/replication/tcp/protocol.py Outdated Show resolved Hide resolved
synapse/replication/tcp/protocol.py Outdated Show resolved Hide resolved

Does not check if there is already a thread processing the queue, hence "unsafe"
"""
self._processing_streams.add(stream_name)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add another guard here to test stream_name in self._processing_streams in case there is a race? I don't think there should be but it requires understanding how run_as_background_process works

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a pattern we use pretty widely across the codebase. I've added an assertion.

@richvdh
Copy link
Member Author

richvdh commented Jul 27, 2020

looks like something has broken here; am investigating.

synapse/replication/tcp/redis.py Outdated Show resolved Hide resolved
Co-authored-by: Erik Johnston <erik@matrix.org>
@richvdh
Copy link
Member Author

richvdh commented Jul 27, 2020

This also fixes a potential assertion failure introduced in #7861:

AssertionError: null
  File "/home/synapse/src/synapse/metrics/background_process_metrics.py", line 214, in run
  File "/home/synapse/env-py37/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
  File "/home/synapse/src/synapse/replication/tcp/redis.py", line 137, in handle_command
  File "/home/synapse/src/synapse/replication/tcp/handler.py", line 387, in on_RDATA
  File "/home/synapse/src/synapse/replication/tcp/handler.py", line 219, in _add_command_to_stream_queue

@richvdh richvdh changed the base branch from develop to release-v1.18.0 July 27, 2020 17:54
@richvdh richvdh merged commit f57b99a into release-v1.18.0 Jul 27, 2020
@richvdh richvdh deleted the rav/synchronous_replication branch July 27, 2020 17:54
richvdh added a commit that referenced this pull request Jul 28, 2020
Synapse 1.18.0rc2 (2020-07-28)
==============================

Bugfixes
--------

- Fix an `AssertionError` exception introduced in v1.18.0rc1. ([\#7876](#7876))
- Fix experimental support for moving typing off master when worker is restarted, which is broken in v1.18.0rc1. ([\#7967](#7967))

Internal Changes
----------------

- Further optimise queueing of inbound replication commands. ([\#7876](#7876))
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '7000a215e':
  1.18.0rc2
  Typing worker needs to handle stream update requests (#7967)
  Handle replication commands synchronously where possible (#7876)
  update changelog
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants