Lay some foundation work to allow workers to only subscribe to some kinds of messages, reducing replication traffic.

[reivilibre]

Some foundation work towards #12463, especially #12460 and #12461.

All workers continue to publish on the unsuffixed Redis channel, so listening to multiple channels is just preparation for the future.
At a future version, we should be able to switch over to publishing some kinds of messages on the suffixed channels and then uninterested workers don't have to subscribe to those.

Before

Channel name: example.com (listen + publish)

After

Channel names:

• example.com (listen + publish)
• example.com/USER_IP (listen only for now)
• example.com/RDATA/events etc (listen only for now)

Review should be commit by commit.

[erikjohnston]

woop!

[erikjohnston]

I'm not a huge fan of hardcoding the list of streams here. Can we either:

1. Leave RDATA on the main channel for now; or
2. Subscribe to RDATA/* and use PSUBSCRIBE

[reivilibre]

Leave RDATA on the main channel for now; or

I think I'd prefer this (mind you, it's still transmitting on the main channel). How do you suggest this work without specifying a list of streams here?
I guess we could have them be subscribed on-demand somewhere else, but I was sort of keen to subscribe them all in one place to be sure we're not sending REPLICATE before we are listening on all the desired channels.

Subscribe to RDATA/* and use PSUBSCRIBE

I am somewhat reluctant to do this because it's not really sorting us out to be able to unsubscribe from select streams in the future (and not to mention that it seems like it's more work for Redis to do pattern matches on each receiver rather than having a concrete set of listeners per channel; not sure how it's implemented in practice though).

[erikjohnston]

It sort of depends on what we want the final code to look like. One option that I've been considering is have it so that every handler that wants to listen to a stream has to call something like ReplicationSubscriber.subscribe_to_stream(Stream.NAME, func) in the handler's __init__. Then, when we come to connect to Redis the ReplicationSubscriber has the list of streams the worker is interested in.

What I really want to avoid is to have to manually list these stream names, its a recipe for it getting out of date and its very non-obvious how it all fits together.

Using PSUBSCRIBE would allow us to set up the channels and write to those channels before putting all the logic in for doing ReplicationSubscriber. OTOH, you could implement the ReplicationSubscriber logic before adding the streams

[reivilibre]

I'll give that a go. Otherwise it may be worth cutting out any mention of RDATA in this PR and leaving that rewiring for another PR.

its very non-obvious how it all fits together.

yeees. That can be said about much of the replication stuff, I don't want to make it much worse though, so will give this a try

[erikjohnston]

Cool. I think its worth time boxing the RDATA stuff, as just getting the UserIP stuff split out will provide a bunch of value.

[reivilibre]

I've de-scoped the RDATA stuff a bit for now (mostly because I'd appreciate a few thoughts on where exactly to hook it in; the code processing RDATA commands is a bit convoluted and I thought it was going to take a while to untangle), but I've restructured things in a way that I think makes it a bit more approachable to do cleanly.

[reivilibre]

RDATA descoped but this still listens properly to USER_IP only when needed:

rei@lithium ~/work/servers/syn7_auto$ egrep 'SUBSCRIBE' logs/synchrotron1.log | tail -n1
2022-05-13 15:21:52,874 - synapse.replication.tcp.redis - 125 - INFO - subscribe-replication-0 - Sending redis SUBSCRIBE for ['syn7']

rei@lithium ~/work/servers/syn7_auto$ egrep 'SUBSCRIBE' logs/background_worker1.log | tail -n1
2022-05-13 15:21:32,978 - synapse.replication.tcp.redis - 125 - INFO - subscribe-replication-0 - Sending redis SUBSCRIBE for ['syn7/USER_IP', 'syn7']

rei@lithium ~/work/servers/syn7_auto$ egrep 'SUBSCRIBE' logs/main.log | tail -n1
2022-05-13 15:21:47,340 - synapse.replication.tcp.redis - 125 - INFO - subscribe-replication-0 - Sending redis SUBSCRIBE for ['syn7/USER_IP', 'syn7']

rei@lithium ~/work/servers/syn7_auto$ egrep 'SUBSCRIBE' logs/federation_reader1.log | tail -n1
2022-05-13 15:21:42,910 - synapse.replication.tcp.redis - 125 - INFO - subscribe-replication-0 - Sending redis SUBSCRIBE for ['syn7']

Switching over the sender shouldn't be too difficult. (Is it OK to do this without a transitional Synapse version in between, since it's 'only the user IPs and IMO it doesn't matter if they get lost for a few moments in a rolling restart'?)

[erikjohnston]

Otherwise lgtm

[erikjohnston]

Can we add a test that checks we do in fact connect to these channels? Presumably we can look at what's in FakeRedisPubSubServer?

[reivilibre]

Good point.

I've added a new base test case class that enables Redis (because even if you're not using it with the main process for anything, you must enable it in order for the Fake Redis server to listen on a fake TCP port) and skips Postgres (since it turns out nothing subscribes to Redis when Postgres is not in use...).