Add experimental support for sharding event persister.

[erikjohnston]

This is not ready for production yet. Caveats:

1. We should write some tests...
2. The stream token that we use for events can get stalled at the minimum position of all writers. This means that new events may not be processed and e.g. sent down sync streams if a writer isn't writing or is slow.

Probably worth looking at this commit by commit. There is a FIXME in Implement config... that is dealt with by the federation handler refactor,

[clokep]

I didn't see anything glaringly incorrect, but did leave quite a few comments. I'm guessing documentation won't be updated until this is not experimental?

[clokep]

Is this if-statement just an optimization?

[erikjohnston]

Oh err, somewhere along the lines I think something got very unhappy about empty lists. I forget the details now or if its even necessary 😕

[clokep]

Edit: I wrote the below before realizing that instance is also used in the _send_events call below, I thought it was worth leaving though in case it jiggles a good idea loose:

It looks like this pattern is used quite a bit, in the comments for ShardedWorkerHandlingConfig it says to prefer should_handle, which seems like it could be used here:

if self.config.worker.events_shard_config.should_handle(self._instance_name, room_id):

Although I think some of this could be simplified more if ShardedWorkerHandlingConfig knew what the current instance was (maybe should_handle wouldn't need the instance passed in?)

[erikjohnston]

nods. The problem with giving ShardedWorkerHandlingConfig the current instance name is that I don't think we know what the instance name is during config parsing (outside of the worker config parsing).

[erikjohnston]

Actually, I think we do need to use should_handle first to technically conform to the docs of ShardedWorkerHandlingConfig

[erikjohnston]

Though I guess the fact that we go on to do a HTTP replication hit means that get_instance has to work.

[clokep]

I think it is OK to use get_instance here, that and should_handle should have matching logic after all!

[erikjohnston]

I'm guessing documentation won't be updated until this is not experimental?

Yeah, I really really don't want people using this at all. Not even in tests (I have a branch with working sytests but they go so slowly)

[clokep]

@erikjohnston So I think this looks good! Not sure if you're looking for a ✅ or want to add tests?

[clokep]

I think this is related to #7986.

[erikjohnston]

@erikjohnston So I think this looks good! Not sure if you're looking for a or want to add tests?

So I was waiting for SyTests to land, but actually it turns out that with this current implementation they take forever. This is because of the get_persisted_position not always advancing due to gaps in the events stream, which can be fixed by making each worker periodically send out updated positions, but a) that makes the tests slow and b) not really something I necessarily want to add.

Given this is completely undocumented I'm tempted to merge as is, and then require tests for the next phase (which is augmenting the get_persisted_position stuff with something a bit smarter)